[sycl-post-link] Set isEsimdImage property for ESIMD modules after splitting #3073
Conversation
| if (ImgPSInfo.IsEsimdKernel) { | ||
| std::map<StringRef, bool> RMEntry = {{"isEsimdImage", true}}; | ||
| PropSet.add(llvm::util::PropertySetRegistry::SYCL_IS_ESIMD_IMAGE, | ||
| RMEntry); | ||
| } |
There was a problem hiding this comment.
This is a whole property set for a single string, boolean pair and as I can see, this is not the first one such set:
llvm/llvm/tools/sycl-post-link/sycl-post-link.cpp
Lines 544 to 549 in 557b564
What do you think about creating a single generic property set for boolean properties? This will slightly reduce amount of code and it will also reduce amount of temporary files created by the compiler
Tagging @kbobrovs here as well
There was a problem hiding this comment.
@AlexeySachkov, I don't feel qualified enough to judge your proposal. I don't know this part of the code very well.
There was a problem hiding this comment.
What do you think about creating a single generic property set for boolean properties?
this seems like a good idea to me with some tweaks. But I think merging this with DeviceLibReqMask would be confusing, as the device lib mask is not a boolean, but a set of booleans. So basically, SYCL/is ESIMD image could just be renamed to something like [SYCL/Binary Image Properties] which could contain all binary image-related properties, not only booleans. Having a bit mask instead of separate boolean properties wouldn't have much perf impact, but would reduce readability, IMO.
it will also reduce amount of temporary files created by the compiler
No sure about his one. @AlexeySachkov - could you elaborate please?
There was a problem hiding this comment.
But I think merging this with DeviceLibReqMask would be confusing, as the device lib mask is not a boolean, but a set of booleans.
Technically that property is absolutely the same as SYCL/is ESIMD image since each property set is a set by its nature, but for llvm::util::PropertySetRegistry::SYCL_DEVICELIB_REQ_MASK we only store a single pair within it - that's why I was referring to it as to a simple boolean property.
Note: I'm not asking to do such refactoring right now in scope of this PR, just highlighted a potential room for improvement
it will also reduce amount of temporary files created by the compiler
No sure about his one. @AlexeySachkov - could you elaborate please?
Please disregard this. By some reason I though that we emit separate .prop file for each propety set, but this is not true - we put all property set related to a particular device image and store then into a single .prop file
There was a problem hiding this comment.
I still think it would be good not to create a separate category per single property. Once out, all categories/property names become legacy which we need to maintain backward compatibility with.
There was a problem hiding this comment.
Maybe 'SYCL/misc' would also work.
CODEOWNERS GitHub feature is not able to correctly assign owners for "only last patch in PR". I don't have a good solution for such case other than to recommend opening a new PR after PR it depends on is merged. Not sure if it make sense to split the implementation in this case. The whole diff is not that big (+39 lines, −3 lines, see #3074) and fist patch in the stack already modifies multiple components. |
Yes, thanks for the recommendation. I understand that making multiple patches that are built on top of each other confuses reviewers. Will hold off subsequent patches in the future or combine them into one as you suggested. |
This change is a part of 3 patches:
Patch (1/3): [new property] #3072
Patch (2/3): [sycl-post-link] this PR
Patch (3/3): [DPCPP RT] #3074
Please review only patch 557b564