Turn on SPIR-V builtin generation for OpenCL CPP sources #2466
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: Sudarsanam, Arvind <[email protected]>
asudarsa
commented
Mar 28, 2024
| !0 = !{i32 1, !"wchar_size", i32 4} | ||
| !1 = !{i32 1, i32 2} | ||
| !2 = !{i32 4, i32 100000} | ||
| !2 = !{i32 7, i32 100000} |
Contributor
Author
There was a problem hiding this comment.
This test case was actually derived from SYCL source.
Signed-off-by: Sudarsanam, Arvind <[email protected]>
Contributor
Author
|
Test fail is not related and an issue has been opened. Thanks |
LU-JOHN
reviewed
Mar 28, 2024
lib/SPIRV/OCLToSPIRV.cpp
Outdated
| // This is a pre-processing pass, which transform LLVM IR module to a more | ||
| // suitable form for the SPIR-V translation: it is specifically designed to | ||
| // handle OpenCL C built-in functions and shouldn't be launched for other | ||
| // handle OpenCL C/CPP built-in functions and shouldn't be launched for other |
Contributor
There was a problem hiding this comment.
Out of curiosity, what is the reason for only running this transform for these types of sources?
Contributor
Author
There was a problem hiding this comment.
These are the only OpenCL related sources and this particular module deals with OpenCL sources.
LU-JOHN
reviewed
Mar 28, 2024
| @@ -0,0 +1,24 @@ | |||
| ; RUN: llvm-as %s -o %t.bc | |||
Contributor
There was a problem hiding this comment.
Consider adding a comment describing purpose of this test. I think that looking at the filename or file history is not as clear or convenient.
LU-JOHN
reviewed
Mar 28, 2024
lib/SPIRV/OCLToSPIRV.cpp
Outdated
| // source languages | ||
| if (std::get<0>(Src) != spv::SourceLanguageOpenCL_C) | ||
| if (std::get<0>(Src) != spv::SourceLanguageOpenCL_C && | ||
| std::get<0>(Src) != spv::SourceLanguageOpenCL_CPP) |
Contributor
There was a problem hiding this comment.
There is also a SourceLanguageCPP_for_OpenCL. Should this be allowed?
Contributor
Author
There was a problem hiding this comment.
Yes. Good catch. Thanks. Added.
Signed-off-by: Sudarsanam, Arvind <[email protected]>
Signed-off-by: Sudarsanam, Arvind <[email protected]>
svenvh
reviewed
Apr 1, 2024
lib/SPIRV/OCLToSPIRV.cpp
Outdated
| // handle OpenCL C built-in functions and shouldn't be launched for other | ||
| // source languages | ||
| if (std::get<0>(Src) != spv::SourceLanguageOpenCL_C) | ||
| // handle OpenCL C/CPP and CPP for OpenCL modules and shouldn't be launched |
Member
There was a problem hiding this comment.
In comments we can refer to the official language names:
Suggested change
| // handle OpenCL C/CPP and CPP for OpenCL modules and shouldn't be launched | |
| // handle OpenCL C/C++ and C++ for OpenCL modules and shouldn't be launched |
Contributor
Author
There was a problem hiding this comment.
Done in commit
Thanks
lib/SPIRV/OCLTypeToSPIRV.cpp
Outdated
| if (std::get<0>(Src) != spv::SourceLanguageOpenCL_C) | ||
| // This is a pre-processing pass, which transform LLVM IR module to a more | ||
| // suitable form for the SPIR-V translation: it is specifically designed to | ||
| // handle OpenCL C/CPP and CPP for OpenCL types and shouldn't be launched for |
Member
There was a problem hiding this comment.
Suggested change
| // handle OpenCL C/CPP and CPP for OpenCL types and shouldn't be launched for | |
| // handle OpenCL C/C++ and C++ for OpenCL types and shouldn't be launched for |
Contributor
Author
There was a problem hiding this comment.
Done in commit. Thanks
Signed-off-by: Arvind Sudarsanam <[email protected]>
svenvh
approved these changes
Apr 2, 2024
bwlodarcz
added a commit
to bwlodarcz/SPIRV-LLVM-Translator
that referenced
this pull request
Apr 18, 2024
…onosGroup#2466)" This reverts commit 80dfd86. The commit break Translator in cases when user supplied function is named with the prefix which, after demangling, is the same as OpenCL builtin. The culprit code is placed in during SPIRWriter OCL pass in OCLToSPIRVBase::visitCallInst(CallInst &). Failing name would be for example atomic_fetch_and_sub_uint32_explicit.
MrSidims
pushed a commit
that referenced
this pull request
Apr 19, 2024
…)" (#2508) This reverts commit 80dfd86. The commit break Translator in cases when user supplied function is named with the prefix which, after demangling, is the same as OpenCL builtin. The culprit code is placed in during SPIRWriter OCL pass in OCLToSPIRVBase::visitCallInst(CallInst &). Failing name would be for example atomic_fetch_and_sub_uint32_explicit.
maarquitos14
added a commit
to maarquitos14/SPIRV-LLVM-Translator
that referenced
this pull request
Feb 27, 2025
…ces (KhronosGroup#2466)" (KhronosGroup#2508)" This reverts commit 9e60105.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
It seems we currently allow OCL to SPIR-V translation only for OpenCL C sources. This PR tries to extend this to OpenCL C++ and C++ for OpenCL sources as well.
Thanks