-
Notifications
You must be signed in to change notification settings - Fork 724
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update spirv.Source. #14593
Update spirv.Source. #14593
Conversation
This aligns with KhronosGroup/SPIRV-LLVM-Translator@a84bfca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know or understand this functionality enough to approve this. Your PR description mentions that this makes us align with KhronosGroup/SPIRV-LLVM-Translator@a84bfca but that seems to be setting spriv.source based on openCL version. How does hard coding 6 here align with it? @bader can you also take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is definitely some mess in here.
SPIR-V spec still mentions OpenCL C++ as one of the source languages for OpSource instruction - https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#Source_Language. Technically, SPIR-V translator should define some LLVM IR level interface to emit this value.
On the other hand, clang never had full support for OpenCL C++ and the spec for this language was removed from Khronos site.
It's not really important for SYCL which value to use for OpSource instruction - it has no semantic impact on the program. Switching from OpenCL C++ to C++ for OpenCL seems okay to me.
To avoid this type of problem, we can add SYCL language to SPIR-V spec. At the same time, I don't see too much value for users in have a separate OpSource for SYCL.
(cc @bashbaug)
@@ -1358,14 +1358,14 @@ void CodeGenModule::Release() { | |||
TheModule.getOrInsertNamedMetadata("opencl.spir.version"); | |||
SPIRVerMD->addOperand(llvm::MDNode::get(Ctx, SPIRVerElts)); | |||
// We are trying to look like OpenCL C++ for SPIR-V translator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The language mode is called "C++ for OpenCL".
// We are trying to look like OpenCL C++ for SPIR-V translator. | |
// We are trying to look like C++ for OpenCL for SPIR-V translator. |
Co-authored-by: Alexey Bader <alexey.bader@intel.com>
Arguably the patch should set the value to |
Yes, set to SYCL is right, but currently spirv sets spirv.Source according to opencl language version. Set to 7 will drop spirv.Source metadata when translate spirv to llvm IR https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/lib/SPIRV/SPIRVReader.cpp#L4834,L4835. |
@intel/dpcpp-spirv-reviewers, is there a problem to add SYCL language support to the SPIRV-LLVM-Translator? |
ping @intel/dpcpp-spirv-reviewers, thanks very much. |
If spirv doesn't drop the SourceLanguageOpenCL_CPP metadata, the PR is not necessary. |
This aligns with KhronosGroup/SPIRV-LLVM-Translator@a84bfca