Support SPV_INTEL_maximum_registers extension #2344
Conversation
|
Please take a look @MrSidims @asudarsa @LU-JOHN @bwlodarcz @VyacheslavLevytskyy |
There was a problem hiding this comment.
LGTM conditionally if tokens are placed in https://github.com/KhronosGroup/SPIRV-Headers
Created PR to Headers, will update the translator code once Headers are merged (can be done in a separate PR) KhronosGroup/SPIRV-Headers#416 |
vmaksimo
changed the title
| @@ -4318,6 +4318,50 @@ bool SPIRVToLLVM::transMetadata() { | |||
| F->setMetadata(kSPIR2MD::IntelFPGAIPInterface, | |||
| MDNode::get(*Context, InterfaceMDVec)); | |||
| } | |||
| if (auto *EM = BF->getExecutionMode( | |||
There was a problem hiding this comment.
Should we check to ensure that only one mode is chosen?
Consider refactoring common code among the three if statements.
There was a problem hiding this comment.
Actually no. Even so it seems to have no sense to enable several different maxregisters execution modes, neither SPIR-V core spec, nor extension prohibits usage of several execution modes at the same time. So, I believe this code should not be changed to else if.
There was a problem hiding this comment.
From KhronosGroup/SPIRV-Registry#235, in extensions/INTEL/SPV_INTEL_maximum_registers.asciidoc
- Each OpEntryPoint must contain at most one of the
MaximumRegistersINTEL, MaximumRegistersIdINTEL, or
NamedMaximumRegistersINTEL execution modes.
There was a problem hiding this comment.
Applied in 84a3ead. Unfortunately I can't add meaningful tests for that, because llvm-spirv fails with an assertion in case we add more than one execution mode of the certain type.
Verified locally that this check both on forward and reverse translation in case of invalid IR/SPV.
| ConstantAsMetadata::get(getUInt32(M, EM->getLiterals()[0]))); | ||
| ExecModeMD->addOperand(MDNode::get(*Context, ValueVec)); | ||
| } | ||
| if (auto *EM = BF->getExecutionMode( |
There was a problem hiding this comment.
Should this be "else if"?
| if (BM->isAllowedToUseExtension(ExtensionID::SPV_INTEL_maximum_registers)) | ||
| transFunctionMetadataAsExecutionMode(BF, F); | ||
| else | ||
| transFunctionMetadataAsUserSemanticDecoration(BF, F); |
There was a problem hiding this comment.
Do we want to support the 'old' implementation or throw an error here? Sorry if this is ignorant question.
There was a problem hiding this comment.
Actually we do, because it takes time for the consumers to adapt the new change on their side.
#2390) Spec: KhronosGroup/SPIRV-Registry#235 * Add infrastructure for translating ExecutionModeId (#2297) This functionality was added in SPIR-V 1.2 and allows using an <id> to set the execution modes SubgroupsPerWorkgroupId, LocalSizeId, and LocalSizeHintI, and others. --------- Co-authored-by: Viktoria Maximova <[email protected]>
#2388) Spec: KhronosGroup/SPIRV-Registry#235 Co-authored-by: Viktoria Maximova <[email protected]>
#2393) Spec: KhronosGroup/SPIRV-Registry#235 Co-authored-by: Viktoria Maximova <[email protected]>
* Add infrastructure for translating ExecutionModeId (#2297) * [Backport to 14] Support SPV_INTEL_maximum_registers extension (#2344) Co-authored-by: Viktoria Maximova <[email protected]>
* Add infrastructure for translating ExecutionModeId (#2297) * [Backport to 13] Support SPV_INTEL_maximum_registers extension (#2344) * .clang-tidy: allow recursion (cherry picked from commit 28fdb7a) Co-authored-by: Viktoria Maximova <[email protected]> Co-authored-by: Sven van Haastregt <[email protected]>
#2400) Co-authored-by: Viktoria Maximova <[email protected]>
#2404) Co-authored-by: Viktoria Maximova <[email protected]>
Spec:
KhronosGroup/SPIRV-Registry#235