Update spirv.Source. #14593
Update spirv.Source. #14593
Conversation
This aligns with KhronosGroup/SPIRV-LLVM-Translator@a84bfca
There was a problem hiding this comment.
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.
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.
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