[SYCL] Fix ESIMD split detection in module properties computation #15527
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
sarnex
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
sarnex
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
sarnex
commented
Sep 27, 2024
| // symbols are usually removed at backend lowering, but this is handled here | ||
| // for SPIR-V since SYCL compilation uses llvm-spirv, not the SPIR-V backend. | ||
| if (auto Triple = M->getTargetTriple().find("spir") != std::string::npos) | ||
| if (M->getTargetTriple().find("spir") != std::string::npos) |
There was a problem hiding this comment.
This is not related to this change but fixes an unused variable warning.
asudarsa
reviewed
Sep 27, 2024
| void ModuleDesc::saveSplitInformationAsMetadata() { | ||
| // Add metadata to the module so we can identify what kind of SYCL/ESIMD split | ||
| // later. | ||
| auto *SplitMD = M->getOrInsertNamedMetadata(SYCL_ESIMD_SPLIT_MD_NAME); |
There was a problem hiding this comment.
A nit:
If we are planning to add more metadata like this in the future, may be it will help to add a helper function?
Not a blocker though
Thanks
There was a problem hiding this comment.
I don't expect it to grow that much, the things we check in module properties is limited and doesn't grow that often. If it does blow up we can just move each MD to a function, sure, thanks.
asudarsa
reviewed
Sep 27, 2024
|
|
||
| namespace module_split { | ||
|
|
||
| constexpr char SYCL_ESIMD_SPLIT_MD_NAME[] = "sycl-split-status"; |
There was a problem hiding this comment.
May be sycl-esimd-split-status?
There was a problem hiding this comment.
sure, will rename, thanks.
asudarsa
approved these changes
Sep 27, 2024
Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
|
thanks for the review! |
sarnex
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
sarnex
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
sarnex
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
sarnex
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We need to know if a module is an ESIMD split or not split at all to set some image properties.
Right now we compute if a module is ESIMD or not by looking at the functions in the module and checking they are all ESIMD, except for allowed exceptions.
A new exception was found related to using
assertin user code where there is a scalar SYCL function that correctly remains in the ESIMD split. The end result is we don't set the ESIMD property because we don't think this is the ESIMD split and do the wrong thing at runtime. Instead of just adding this new exception to the list, I reworked what I consider to be flaky logic (that I wrote originally, whoops) to figure out the splits.Just save metadata in the module of what kind of split it is before we try to compute module properties.
We already do something similar for the spec constant default split, and I moved that to a centralized place as part of this change.
The reason we don't just pass in the
ModuleDescobject as an argument is because we want to untie properties creation from thesycl-post-linktool so that it can be called in other places (which we will do for thinLTO).