-
Notifications
You must be signed in to change notification settings - Fork 734
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
[SYCL][NATIVECPU] Move pipeline creation to CodeGen #12854
[SYCL][NATIVECPU] Move pipeline creation to CodeGen #12854
Conversation
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 am not familiar with the functionality being modified in this PR. I defer to other reviewers for their expertise.
@@ -1087,6 +1087,10 @@ void EmitAssemblyHelper::RunOptimizationPipeline( | |||
|
|||
if (SYCLNativeCPUBackend) { | |||
llvm::sycl::utils::addSYCLNativeCPUBackendPasses(MPM, MAM, Level); | |||
// Run optimization passes after all the changes we made to the kernels. | |||
// Todo: maybe we could find a set of relevant passes instead of re-running |
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.
Nit: TODO s are usually capitalized.
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.
Left a suggestion to improve a comment, but this should not hold up this PR.
// Run optimization passes after all the changes we made to the kernels. | ||
// Todo: maybe we could find a set of relevant passes instead of re-running | ||
// the full optimization pipeline. | ||
MPM.addPass(PB.buildPerModuleDefaultPipeline(Level)); |
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.
Minor (Does not block the PR): It could perhaps be helpful to mention in the comment that the invocation of the pipeline had to be moved into core clang because in the original place in SYCLLowerIR
it introduces a circular dependency that causes problems when building as shared library.
Fixes post commit failure from #12659.