[SYCL] Add support for multiple filtered outputs in sycl-post-link #12727
Conversation
| const module_split::ModuleDesc &M, | ||
| std::map<StringRef, util::PropertyValue> &Requirements); | ||
| struct SYCLDeviceRequirements { | ||
| SYCLDeviceRequirements(const module_split::ModuleDesc &M); |
There was a problem hiding this comment.
nit. I would propose a separate function getSYCLDeviceRequirements or createSYCLDeviceRequirements instead of the sophisticated constructor.
|
@intel/dpcpp-clang-driver-reviewers FYI, can this get a review? @intel/dpcpp-esimd-reviewers Looks like I also need a review from you all, I think |
| std::map<StringRef, util::PropertyValue> Requirements; | ||
|
|
||
| Requirements["aspects"] = | ||
| std::vector<uint32_t>(Aspects.begin(), Aspects.end()); |
There was a problem hiding this comment.
What if Aspects set is empty? Shouldn't we apply if (<property is set>) <fill Requirements> here as it's done for other properties?
There was a problem hiding this comment.
I was preserving the behavior of the code before I refactored this part, which always attached the property. I believe the runtime can handle the property not being attach but I also might need to update some tests if I do.
llvm/llvm/tools/sycl-post-link/SYCLDeviceRequirements.cpp
Lines 64 to 68 in ea7ba1b
| if (!Reqs.ReqdWorkGroupSize.has_value()) | ||
| Reqs.ReqdWorkGroupSize = NewReqdWorkGroupSize; |
There was a problem hiding this comment.
I think we need else branch here to validate consistency of multiple "reqd_work_group_size" requirements.
There was a problem hiding this comment.
If we do this, then this test fails:
Since splitting was disabled, there will be multiple reqd_work_group_size attributes in a module, and then it'll run into an assert if we placed on there. I think the surrounding code that calls getSYCLDeviceRequirements would need to be updated to not compute device requirements when splitting is disabled if we were to add an assert here.
|
@jzc, could you please make a merge with |
AlexeySachkov
dismissed
maksimsab’s stale review
All comments were addressed and review was re-requested a while ago. The patch is approved by another codeowner in the same area. Any further feedback can be applied post-commit, dismissing the review to make progress here
The `sycl_aspects` metadata kept track of the aspect names and their enum values as defined by [sycl/include/sycl/info/aspects.def](https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/info/aspects.def). This metadata was populated by the frontend by looking for the declaration of the SYCL aspects enum. The `CleanupSYCLMetadata` pass then deletes this metadata after device compilation. https://github.com/intel/llvm/blob/f6e73e8953e848ad87259798e9c861e9d44e93b0/clang/lib/CodeGen/BackendUtil.cpp#L1156-L1158 However, some processing in `sycl-post-link` that will be added in #12727 needs the `sycl_aspects` metadata (which at that point, the metadata is already deleted by `CleanupSYCLMetadata`). To fix this, in this PR, 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.
This PR adds the required changes to
sycl-post-linksupport optional kernel features in AOT mode as described by the design doc in #12252. Additionally, it also updates the driver to invokesycl-post-linkwith a device architecture when Intel GPU targets are passed in-fsycl-targets.