llvm
llvm copied to clipboard
intel
[SYCL] Add aspect names to sycl_used_aspects before cleaning up
This PR provides an alternate approach to the problem stated in #13194. Before cleaning up the metadata in CleanupSYCLMetadataPass, we update sycl_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.
https://github.com/intel/llvm/blob/cb2eede6a947e7a5dad221d0056b9f08a68aa931/clang/lib/CodeGen/CGSYCLRuntime.h#L31-L37
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 CleanupSYCLMetadataPass is the right place for it, though. For me, cleanup means removal and suggested changes instead add something new instead of removing stuff. Perhaps a separate dedicated pass for this would be better? Or maybe we can even make it a part of existing propagate aspects usage pass somehow? If we go with the latter, we should be careful, because we are running it twice and the metadata format change should only happen once and ideally not in-between two pass invocations. @steffenlarsen, @intel/dpcpp-tools-reviewers, any feedback here?
@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.
@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.
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.
@intel/dpcpp-esimd-reviewers, could you please take a look? I think it is only CMakeLists.txt change
@intel/llvm-reviewers-runtime, could you please take a look? I think it is only some test changes