Implements spirv.Decorations and spirv.ParameterDecorations metadata translation #1346
Conversation
…translation This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
Signed-off-by: Steffen Larsen <[email protected]>
| if (auto *GV = dyn_cast<GlobalVariable>(V)) { | ||
| std::vector<SPIRVDecorate const *> Decorates = BV->getDecorations(); | ||
| if (!Decorates.empty()) { | ||
| MDNode *MDList = transDecorationsToMetadataList(Context, Decorates); |
There was a problem hiding this comment.
It does look like, that in the end after reverse translation we will have both decoration metadata and it's previously defined representation in LLVM IR module, correct? If so, it kind of looking incorrect, since SPIR-V friendly LLVM IR should be only emitted under SPV-IR flag (and decoration metadata is SPIR-V friendly form). But at the same time, I'm 100% sure, that none of SPIR-V centric LLVM IR device backends are ready to switch from usual representation to decoration metadata representation, so I believe, we should leave this code as is, but with some comment added, WDYT?
There was a problem hiding this comment.
If I'm correct about duality of LLVM IR output, could you please reflect it in the tests?
There was a problem hiding this comment.
You're absolutely right! It didn't seem like a lot of extra work to only generate the metadata if in SPIR-V friendly mode so I went with that instead. The tests have also been changed to check that the metadata is only there if SPV-IR mode is set. Is this okay or would you prefer I revert it and just add a comment?
There was a problem hiding this comment.
I'd gather opinions from @aratajew @svenvh and @AlexeySotkin
Folks, like this in SPIR-V friendly IR after the translation !spirv.(Param)Decoration will appear and will replace previous representation of some features, for example kernel_arg_type_quals carrying volatile/restrict qualifiers will replaced with the mentioned SPIR-V friendly form. Even if I believe, that it's the correct way to go, I'm afraid, that right now it might break gathering of Arg type qualifiers in OpenCL runtime, since !spirv.(Param)Decoration metadata aren't supported in any devices' compilers yet. WDYT?
There was a problem hiding this comment.
In my opinion, !spirv.(Param)Decoration metadata representation should be used only when a user triggers translation with --spirv-target-env=SPV-IR parameter. When --spirv-target-env=CL2.0 or --spirv-target-env=CL1.2 is specified, then SPIRVToOCL pass should translate !spirv.(Param)Decoration to OpenCL representation (kernel_arg_type_quals etc.). As I can see, this PR doesn't include any changes in SPIRVToOCL pass, so I agree with @MrSidims that this change will break OpenCL backends that are based CL2.0 or CL1.2 translation.
Also, just out of curiousity, @steffenlarsen what is the main motivation to use !spirv.(Param)Decoration for SPIR-V Friendly IR path instead of OpenCL representation? Is it because not all SPIRV decorations have their's corresponding OpenCL metadata?
There was a problem hiding this comment.
In my opinion, !spirv.(Param)Decoration metadata representation should be used only when a user triggers translation with --spirv-target-env=SPV-IR parameter. When --spirv-target-env=CL2.0 or --spirv-target-env=CL1.2 is specified, then SPIRVToOCL pass should translate !spirv.(Param)Decoration to OpenCL representation (kernel_arg_type_quals etc.). As I can see, this PR doesn't include any changes in SPIRVToOCL pass, so I agree with @MrSidims that this change will break OpenCL backends that are based CL2.0 or CL1.2 translation.
I see that I misunderstood the concern. This change properly produces metadata based on --spirv-target, but the concern is whether OpenCL Runtimes based on SPV-IR translation path will be able to consume new metadata representation. In case of Intel OpenCL GPU backend, the compiler is in the middle of transition from internal SPIRVReader to Khronos one, so there will be a need to implement a support for new metadata.
There was a problem hiding this comment.
Also, just out of curiousity, @steffenlarsen what is the main motivation to use !spirv.(Param)Decoration for SPIR-V Friendly IR path instead of OpenCL representation? Is it because not all SPIRV decorations have their's corresponding OpenCL metadata?
Yes, that is definitely a driving factor. !spirv.(Param)Decoration offers more freedom at the cost of making it more SPIR-V specific. In principle any implementation could implement some extension decorator that the translator does not yet support and still be able to produce SPIR-V using that decorator by generating !spirv.(Param)Decoration, assuming the parameter space is simple. Additionally, I personally think there is value to the generality of !spirv.(Param)Decoration, i.e. the parsing of the metadata trees are overlapping for the two current variants and future versions would likely have the same structure.
Signed-off-by: Steffen Larsen <[email protected]>
Signed-off-by: Steffen Larsen <[email protected]>
|
@MrSidims @aratajew @svenvh @AlexeySotkin - Friendly ping. Are there any outstanding concerns or comments for this? |
| // Decoration metadata is only enabled in SPIR-V friendly mode | ||
| if (BM->getDesiredBIsRepresentation() == BIsRepresentation::SPIRVFriendlyIR) | ||
| transVarDecorationsToMetadata(BV, V); |
There was a problem hiding this comment.
This 'if' might be removed in the future, making the Decoration metadata be emitted by default and be transformed to OpenCL representation in SPIRVToOCL pass, but lets first see new features being developed atop of this infrastructure.
|
Something went wrong after merging this patch: |
…translation (KhronosGroup#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…rations metadata translation (KhronosGroup#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…rations metadata translation (KhronosGroup#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…ations metadata translation (KhronosGroup#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…ations metadata translation (KhronosGroup#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…rations metadata translation (KhronosGroup#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…rations metadata translation (#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…ations metadata translation (#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…rations metadata translation (KhronosGroup#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
…rations metadata translation (#1346) This patch adds translation to and from `spirv.Decorations` and `spirv.ParameterDecorations` metadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR. Signed-off-by: Steffen Larsen <[email protected]>
This patch adds translation to and from
spirv.Decorationsandspirv.ParameterDecorationsmetadata. These metadata nodes represent global variable and function parameter decorations respectively, allowing explicit decoration in LLVM IR.Documentation: #1319