-
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] Add aspect names to sycl_used_aspects before cleaning up #13486
Conversation
Conceptually that is a cool idea, I think. It allows us to avoid potentially expensive FE changes to generate aspect names when aspects are attached through the corresponding attribute. It also shouldn't cause significant LLVM IR size increase, because even if the same aspect is used by different functions, metadata would just point to the same node for it, without duplicating the string with the aspect name. I'm not entirely sure if |
@AlexeySachkov I did consider making it a part of the propagate aspect pass, but I did forget about the two pass approach. I was originally thinking that doing it in that pass would cause a lot of tests to have their format changed. However, thinking about it more and now considering the two pass approach, I think if we wanted to do this transformation in the propagate aspects pass, we could a parameter to the pass that indicates whether this transformation is applied. That way, we could have the second pass do the transformation and not in-between. With the parameter too, I think it'll also reduce the amount of tests we have to change if we make the original behavior the default behavior of the pass. |
@@ -980,7 +980,29 @@ getDeviceCodeSplitter(ModuleDesc &&MD, IRSplitMode Mode, bool IROutputOnly, | |||
// output files in existing tests. | |||
Categorizer.registerSimpleStringAttributeRule("sycl-register-alloc-mode"); | |||
Categorizer.registerSimpleStringAttributeRule("sycl-grf-size"); | |||
Categorizer.registerListOfIntegersInMetadataSortedRule("sycl_used_aspects"); | |||
Categorizer.registerRule([&](Function *F) { |
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.
Please, move it to dedicated function.
Note that my preference is probably to have a dedicated pass for this transformation just to have cleaner and more modular implementation even though the pass will likely be very small. |
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.
The FE changes themselves are small, I am okay with it.
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.
FE change LGTM
@intel/dpcpp-esimd-reviewers, could you please take a look? I think it is only |
After #13486, aspect name information is visible in `sycl-post-link` without the use of `!sycl_aspects`, so this PR updates `sycl-post-link` to use the aspect names that are now available within the `!sycl_used_aspects` metadata instead of `!sycl_aspects`. Aditionally, this PR also adds E2E related to optional kernel features for AOT enabled by these changes
…13974) After intel#13486, aspect name information is visible in `sycl-post-link` without the use of `!sycl_aspects`, so this PR updates `sycl-post-link` to use the aspect names that are now available within the `!sycl_used_aspects` metadata instead of `!sycl_aspects`. Aditionally, this PR also adds E2E related to optional kernel features for AOT enabled by these changes
This PR provides an alternate approach to the problem stated in #13194. Before cleaning up the metadata in
CleanupSYCLMetadataPass
, we updatesycl_used_aspects
metadata to include the aspect name. Note that the aspect name might not exist, e.g. in the case of the negative-valued "internal" aspects, in which case the value is passed along as normal.llvm/clang/lib/CodeGen/CGSYCLRuntime.h
Lines 31 to 37 in cb2eede