-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MLIR] Implement tuning - step 3: bwd, nonxdlops + xdlops #1152
Conversation
82964d5
to
a000cfc
Compare
This comment has been minimized.
This comment has been minimized.
construction_parameters.kernel_file = construction_parameters.kernel_name + ".mlir"; | ||
|
||
if(config == PerformanceConvMlirIgemmXdlops()) | ||
// At this case, do not pass in the invalid perf config and instead make Miir library to do | ||
// heuristic initialization | ||
construction_parameters.comp_options = | ||
mlir::ConstructBuildOptions(ctx, GetOperation(), GetKernelName(), true); | ||
construction_parameters.comp_options = mlir::ConstructBuildOptions(ctx, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetPerformanceConfig()
must return valid performance config.
PerformanceConvMlirIgemm ConvMlirIgemmBwd::GetPerformanceConfig(const ConvolutionContext& ctx) const | ||
{ | ||
std::ignore = ctx; | ||
return {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This produces invalid performance config, which does not match the specification. Why this is done in GetSoluiton
but not in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @jerryyin to check. Sorry to have missed it in last review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it break the existing specification, but this is intentional. I will explain why I made this decision in #1154 in case this thread becomes too long. Then based on the discussion result, we can decide what's the best behavior it is for mlir solvers.
Change description
PerformanceConvMlirIgemm
for bwd solverPerformanceConvMlirIgemmXdlops
for bwd xdlops solvermlir_common.cpp
GetKernelName()
andGetOperation()
into it to make it solver independentTest on
ConvMlirIgemmBwd
Command:
Log:
Test on
ConvMlirIgemmBwdXdlops
Command:
Log:
Previous PR this series: