[SYCL][NVPTX][NFC] Refactor NVPTX target configuration by bader · Pull Request #3535 · intel/llvm (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits4 Checks0 Files changed
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
[ Show hidden characters]({{ revealButtonHref }})
Avoid using -sycldevice for configuring NVPTX target.
Avoid using -sycldevice for configuring NVPTX target.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other changes look okay to me. Could you add tests for this if applicable?
| @@ -48,7 +48,9 @@ class LocalAccessorToSharedMemory : public ModulePass { |
|---|
| // Access `nvvm.annotations` to determine which functions are kernel entry |
| // points. |
| auto NvvmMetadata = M.getNamedMetadata("nvvm.annotations"); |
| assert(NvvmMetadata && "IR compiled to PTX must have nvvm.annotations"); |
| if (!NvvmMetadata) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a related change?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We skipped this pass for modules w/o -sycldevice triple component in llvm/lib/Target/NVPTX/NVPTXTargetMachine.cpp and now this pass is executed regardless of the environment component value. SYCL compiler always added nvvm.annotations metadata, but it might be missing. In case when this metadata is not emitted, we just skip the transformation instead of crashing (with enabled assertions).
[SYCL][NVPTX] Refactor NVPTX target configuration [SYCL][NVPTX][NFC] Refactor NVPTX target configuration
Could you add tests for this if applicable?
This patch doesn't add new functionality - it's a refactoring of existing features, so I think regression tests we already have should be enough.
Could you add tests for this if applicable?
This patch doesn't add new functionality - it's a refactoring of existing features, so I think regression tests we already have should be enough.
Thanks for explaining. But given the potential for affecting (one of your TODOs) non-sycl kernels, I would remove the [NFC] from the title, just to not mislead.
