From ce46482db7ab3ea9c52fce832d27ca40b14f8e87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Thu, 6 Jun 2024 12:17:51 +0200 Subject: [PATCH] Add KHR suffix to OpExtInstWithForwardRef opcode. (#5704) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The KHR suffix was missing from the published SPIR-V extension. This is now fixed, but requires some patches in SPIRV-Tools. Signed-off-by: Nathan Gauër --- DEPS | 2 +- source/opcode.cpp | 2 +- source/opcode.h | 2 +- source/operand.cpp | 4 ++-- source/opt/ir_context.h | 5 +++-- source/val/validate_adjacency.cpp | 2 +- source/val/validate_decorations.cpp | 2 +- source/val/validate_id.cpp | 11 ++++++----- source/val/validate_layout.cpp | 4 ++-- source/val/validation_state.cpp | 2 +- test/val/val_ext_inst_test.cpp | 25 +++++++++++++------------ test/val/val_extensions_test.cpp | 9 +++++---- test/val/val_id_test.cpp | 13 +++++++------ 13 files changed, 44 insertions(+), 39 deletions(-) diff --git a/DEPS b/DEPS index 3ecaba5b24..5c5b2ee26c 100644 --- a/DEPS +++ b/DEPS @@ -14,7 +14,7 @@ vars = { 're2_revision': '917047f3606d3ba9e2de0d383c3cd80c94ed732c', - 'spirv_headers_revision': 'fbf2402969ed9aec34a8f8b4f5afab342319f07b', + 'spirv_headers_revision': 'eb49bb7b1136298b77945c52b4bbbc433f7885de', } deps = { diff --git a/source/opcode.cpp b/source/opcode.cpp index c28f26bf70..5076bbddc2 100644 --- a/source/opcode.cpp +++ b/source/opcode.cpp @@ -723,7 +723,7 @@ bool spvOpcodeIsImageSample(const spv::Op opcode) { bool spvIsExtendedInstruction(const spv::Op opcode) { switch (opcode) { case spv::Op::OpExtInst: - case spv::Op::OpExtInstWithForwardRefs: + case spv::Op::OpExtInstWithForwardRefsKHR: return true; default: return false; diff --git a/source/opcode.h b/source/opcode.h index 6578e80845..cecd566330 100644 --- a/source/opcode.h +++ b/source/opcode.h @@ -146,7 +146,7 @@ bool spvOpcodeIsLinearAlgebra(spv::Op opcode); // Returns true for opcodes that represent image sample instructions. bool spvOpcodeIsImageSample(spv::Op opcode); -// Returns true if the opcode is either OpExtInst or OpExtInstWithForwardRefs +// Returns true if the opcode is either OpExtInst or OpExtInstWithForwardRefsKHR bool spvIsExtendedInstruction(spv::Op opcode); // Returns a vector containing the indices of the memory semantics diff --git a/source/operand.cpp b/source/operand.cpp index dc80588219..e15004541b 100644 --- a/source/operand.cpp +++ b/source/operand.cpp @@ -584,10 +584,10 @@ std::function spvOperandCanBeForwardDeclaredFunction( std::function spvDbgInfoExtOperandCanBeForwardDeclaredFunction( spv::Op opcode, spv_ext_inst_type_t ext_type, uint32_t key) { // The Vulkan debug info extended instruction set is non-semantic so allows no - // forward references except if used through OpExtInstWithForwardRefs. + // forward references except if used through OpExtInstWithForwardRefsKHR. if (ext_type == SPV_EXT_INST_TYPE_NONSEMANTIC_SHADER_DEBUGINFO_100) { return [opcode](unsigned) { - return opcode == spv::Op::OpExtInstWithForwardRefs; + return opcode == spv::Op::OpExtInstWithForwardRefsKHR; }; } diff --git a/source/opt/ir_context.h b/source/opt/ir_context.h index 6e1713cdfb..3857696618 100644 --- a/source/opt/ir_context.h +++ b/source/opt/ir_context.h @@ -202,8 +202,9 @@ class IRContext { inline IteratorRange debugs3() const; // Iterators for debug info instructions (excluding OpLine & OpNoLine) - // contained in this module. These are OpExtInst & OpExtInstWithForwardRefs - // for DebugInfo extension placed between section 9 and 10. + // contained in this module. These are OpExtInst & + // OpExtInstWithForwardRefsKHR for DebugInfo extension placed between section + // 9 and 10. inline Module::inst_iterator ext_inst_debuginfo_begin(); inline Module::inst_iterator ext_inst_debuginfo_end(); inline IteratorRange ext_inst_debuginfo(); diff --git a/source/val/validate_adjacency.cpp b/source/val/validate_adjacency.cpp index 013c167f3b..e6b00424d4 100644 --- a/source/val/validate_adjacency.cpp +++ b/source/val/validate_adjacency.cpp @@ -52,7 +52,7 @@ spv_result_t ValidateAdjacency(ValidationState_t& _) { adjacency_status == IN_NEW_FUNCTION ? IN_ENTRY_BLOCK : PHI_VALID; break; case spv::Op::OpExtInst: - case spv::Op::OpExtInstWithForwardRefs: + case spv::Op::OpExtInstWithForwardRefsKHR: // If it is a debug info instruction, we do not change the status to // allow debug info instructions before OpVariable in a function. // TODO(https://gitlab.khronos.org/spirv/SPIR-V/issues/533): We need diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp index 13b2aa2bf4..7364cab7ae 100644 --- a/source/val/validate_decorations.cpp +++ b/source/val/validate_decorations.cpp @@ -1684,7 +1684,7 @@ spv_result_t CheckIntegerWrapDecoration(ValidationState_t& vstate, case spv::Op::OpSNegate: return SPV_SUCCESS; case spv::Op::OpExtInst: - case spv::Op::OpExtInstWithForwardRefs: + case spv::Op::OpExtInstWithForwardRefsKHR: // TODO(dneto): Only certain extended instructions allow these // decorations. For now allow anything. return SPV_SUCCESS; diff --git a/source/val/validate_id.cpp b/source/val/validate_id.cpp index d7d3e8f4fc..23512125d0 100644 --- a/source/val/validate_id.cpp +++ b/source/val/validate_id.cpp @@ -178,7 +178,7 @@ spv_result_t IdPass(ValidationState_t& _, Instruction* inst) { !inst->IsNonSemantic() && !spvOpcodeIsDecoration(opcode) && !spvOpcodeIsBranch(opcode) && opcode != spv::Op::OpPhi && opcode != spv::Op::OpExtInst && - opcode != spv::Op::OpExtInstWithForwardRefs && + opcode != spv::Op::OpExtInstWithForwardRefsKHR && opcode != spv::Op::OpExtInstImport && opcode != spv::Op::OpSelectionMerge && opcode != spv::Op::OpLoopMerge && @@ -239,10 +239,10 @@ spv_result_t IdPass(ValidationState_t& _, Instruction* inst) { // defined" error before we could give a more helpful message. For this // reason, this test is done here, so we can be more helpful to the // user. - if (inst->opcode() == spv::Op::OpExtInstWithForwardRefs && + if (inst->opcode() == spv::Op::OpExtInstWithForwardRefsKHR && !inst->IsNonSemantic()) return _.diag(SPV_ERROR_INVALID_DATA, inst) - << "OpExtInstWithForwardRefs is only allowed with " + << "OpExtInstWithForwardRefsKHR is only allowed with " "non-semantic instructions."; ret = SPV_SUCCESS; break; @@ -253,10 +253,11 @@ spv_result_t IdPass(ValidationState_t& _, Instruction* inst) { if (SPV_SUCCESS != ret) return ret; } const bool must_have_forward_declared_ids = - inst->opcode() == spv::Op::OpExtInstWithForwardRefs; + inst->opcode() == spv::Op::OpExtInstWithForwardRefsKHR; if (must_have_forward_declared_ids && !has_forward_declared_ids) { return _.diag(SPV_ERROR_INVALID_ID, inst) - << "Opcode OpExtInstWithForwardRefs must have at least one forward " + << "Opcode OpExtInstWithForwardRefsKHR must have at least one " + "forward " "declared ID."; } diff --git a/source/val/validate_layout.cpp b/source/val/validate_layout.cpp index 3efeea62fe..05a8675101 100644 --- a/source/val/validate_layout.cpp +++ b/source/val/validate_layout.cpp @@ -35,7 +35,7 @@ spv_result_t ModuleScopedInstructions(ValidationState_t& _, const Instruction* inst, spv::Op opcode) { switch (opcode) { case spv::Op::OpExtInst: - case spv::Op::OpExtInstWithForwardRefs: + case spv::Op::OpExtInstWithForwardRefsKHR: if (spvExtInstIsDebugInfo(inst->ext_inst_type())) { const uint32_t ext_inst_index = inst->word(4); bool local_debug_info = false; @@ -244,7 +244,7 @@ spv_result_t FunctionScopedInstructions(ValidationState_t& _, break; case spv::Op::OpExtInst: - case spv::Op::OpExtInstWithForwardRefs: + case spv::Op::OpExtInstWithForwardRefsKHR: if (spvExtInstIsDebugInfo(inst->ext_inst_type())) { const uint32_t ext_inst_index = inst->word(4); bool local_debug_info = false; diff --git a/source/val/validation_state.cpp b/source/val/validation_state.cpp index e4c7014f79..b5ed65fc25 100644 --- a/source/val/validation_state.cpp +++ b/source/val/validation_state.cpp @@ -76,7 +76,7 @@ ModuleLayoutSection InstructionLayoutSection( if (current_section == kLayoutTypes) return kLayoutTypes; return kLayoutFunctionDefinitions; case spv::Op::OpExtInst: - case spv::Op::OpExtInstWithForwardRefs: + case spv::Op::OpExtInstWithForwardRefsKHR: // spv::Op::OpExtInst is only allowed in types section for certain // extended instruction sets. This will be checked separately. if (current_section == kLayoutTypes) return kLayoutTypes; diff --git a/test/val/val_ext_inst_test.cpp b/test/val/val_ext_inst_test.cpp index 30f25a5b67..996b9dabc4 100644 --- a/test/val/val_ext_inst_test.cpp +++ b/test/val/val_ext_inst_test.cpp @@ -7492,12 +7492,12 @@ TEST_F(ValidateExtInst, OpExtInstWithForwardNotAllowedSemantic) { %7 = OpTypeFunction %void %8 = OpExtInst %void %1 DebugSource %3 %3 %9 = OpExtInst %void %1 DebugCompilationUnit %uint_0 %uint_0 %8 %uint_0 - %10 = OpExtInstWithForwardRefs %void %1 DebugTypeFunction %uint_0 %11 - %12 = OpExtInstWithForwardRefs %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 + %10 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeFunction %uint_0 %11 + %12 = OpExtInstWithForwardRefsKHR %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 %11 = OpExtInst %void %1 DebugTypeComposite %3 %uint_0 %8 %uint_0 %uint_0 %9 %3 %uint_0 %uint_0 %12 %2 = OpFunction %void None %7 %13 = OpLabel - %18 = OpExtInstWithForwardRefs %f32 %extinst FMin %f32_0 %19 + %18 = OpExtInstWithForwardRefsKHR %f32 %extinst FMin %f32_0 %19 %19 = OpExtInst %f32 %extinst FMin %f32_0 %f32_1 OpReturn OpFunctionEnd @@ -7508,9 +7508,9 @@ TEST_F(ValidateExtInst, OpExtInstWithForwardNotAllowedSemantic) { EXPECT_THAT( getDiagnosticString(), HasSubstr( - "OpExtInstWithForwardRefs is only allowed with non-semantic " + "OpExtInstWithForwardRefsKHR is only allowed with non-semantic " "instructions.\n" - " %18 = OpExtInstWithForwardRefs %float %2 FMin %float_0 %19\n")); + " %18 = OpExtInstWithForwardRefsKHR %float %2 FMin %float_0 %19\n")); } TEST_F(ValidateExtInst, OpExtInstRequiresNonSemanticBefore16) { @@ -7529,8 +7529,8 @@ TEST_F(ValidateExtInst, OpExtInstRequiresNonSemanticBefore16) { %7 = OpTypeFunction %void %8 = OpExtInst %void %1 DebugSource %3 %3 %9 = OpExtInst %void %1 DebugCompilationUnit %uint_0 %uint_0 %8 %uint_0 - %10 = OpExtInstWithForwardRefs %void %1 DebugTypeFunction %uint_0 %11 - %12 = OpExtInstWithForwardRefs %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 + %10 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeFunction %uint_0 %11 + %12 = OpExtInstWithForwardRefsKHR %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 %11 = OpExtInst %void %1 DebugTypeComposite %3 %uint_0 %8 %uint_0 %uint_0 %9 %3 %uint_0 %uint_0 %12 %2 = OpFunction %void None %7 %13 = OpLabel @@ -7540,11 +7540,12 @@ TEST_F(ValidateExtInst, OpExtInstRequiresNonSemanticBefore16) { CompileSuccessfully(body); ASSERT_EQ(SPV_ERROR_MISSING_EXTENSION, ValidateInstructions()); - EXPECT_THAT(getDiagnosticString(), - HasSubstr("ExtInstWithForwardRefs requires one of the following " - "extensions: SPV_KHR_relaxed_extended_instruction \n" - " %11 = OpExtInstWithForwardRefs %void %1 " - "DebugTypeFunction %uint_0 %12\n")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("ExtInstWithForwardRefsKHR requires one of the following " + "extensions: SPV_KHR_relaxed_extended_instruction \n" + " %11 = OpExtInstWithForwardRefsKHR %void %1 " + "DebugTypeFunction %uint_0 %12\n")); } } // namespace diff --git a/test/val/val_extensions_test.cpp b/test/val/val_extensions_test.cpp index bdeb3293c7..bc8e9728ae 100644 --- a/test/val/val_extensions_test.cpp +++ b/test/val/val_extensions_test.cpp @@ -568,8 +568,8 @@ TEST_F(ValidateRelaxedExtendedInstructionExt, RequiresExtension) { %7 = OpTypeFunction %void %8 = OpExtInst %void %1 DebugSource %3 %3 %9 = OpExtInst %void %1 DebugCompilationUnit %uint_0 %uint_0 %8 %uint_0 - %10 = OpExtInstWithForwardRefs %void %1 DebugTypeFunction %uint_0 %11 - %12 = OpExtInstWithForwardRefs %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 + %10 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeFunction %uint_0 %11 + %12 = OpExtInstWithForwardRefsKHR %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 %11 = OpExtInst %void %1 DebugTypeComposite %3 %uint_0 %8 %uint_0 %uint_0 %9 %3 %uint_0 %uint_0 %12 %2 = OpFunction %void None %7 %13 = OpLabel @@ -582,9 +582,10 @@ TEST_F(ValidateRelaxedExtendedInstructionExt, RequiresExtension) { EXPECT_THAT( getDiagnosticString(), HasSubstr( - "ExtInstWithForwardRefs requires one of the following extensions:" + "ExtInstWithForwardRefsKHR requires one of the following extensions:" " SPV_KHR_relaxed_extended_instruction \n" - " %10 = OpExtInstWithForwardRefs %void %1 DebugTypeFunction %uint_0 " + " %10 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeFunction " + "%uint_0 " "%11\n")); } diff --git a/test/val/val_id_test.cpp b/test/val/val_id_test.cpp index c4e25620e4..c1b8e41f94 100644 --- a/test/val/val_id_test.cpp +++ b/test/val/val_id_test.cpp @@ -6907,7 +6907,8 @@ TEST_P(ValidateIdWithMessage, NVBindlessSamplerInStruct) { EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_UNIVERSAL_1_3)); } -TEST_P(ValidateIdWithMessage, OpExtInstWithForwardRefsDisallowedNoForwardRef) { +TEST_P(ValidateIdWithMessage, + OpExtInstWithForwardRefsKHRDisallowedNoForwardRef) { std::string spirv = R"( OpCapability Shader OpExtension "SPV_KHR_non_semantic_info" @@ -6918,7 +6919,7 @@ TEST_P(ValidateIdWithMessage, OpExtInstWithForwardRefsDisallowedNoForwardRef) { OpExecutionMode %main LocalSize 1 1 1 %void = OpTypeVoid %main_type = OpTypeFunction %void - %4 = OpExtInstWithForwardRefs %void %1 DebugInfoNone + %4 = OpExtInstWithForwardRefsKHR %void %1 DebugInfoNone %main = OpFunction %void None %main_type %5 = OpLabel OpReturn @@ -6929,7 +6930,7 @@ TEST_P(ValidateIdWithMessage, OpExtInstWithForwardRefsDisallowedNoForwardRef) { EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_6)); EXPECT_THAT( getDiagnosticString(), - HasSubstr(make_message("Opcode OpExtInstWithForwardRefs must have at " + HasSubstr(make_message("Opcode OpExtInstWithForwardRefsKHR must have at " "least one forward declared ID."))); } @@ -6956,7 +6957,7 @@ TEST_P(ValidateIdWithMessage, OpExtInstNoForwardRef) { } TEST_P(ValidateIdWithMessage, - OpExtInstWithForwardRefsAllowedForwardReferenceInNonSemantic) { + OpExtInstWithForwardRefsKHRAllowedForwardReferenceInNonSemantic) { std::string spirv = R"( OpCapability Shader OpExtension "SPV_KHR_non_semantic_info" @@ -6972,8 +6973,8 @@ TEST_P(ValidateIdWithMessage, %7 = OpTypeFunction %void %8 = OpExtInst %void %1 DebugSource %3 %3 %9 = OpExtInst %void %1 DebugCompilationUnit %uint_0 %uint_0 %8 %uint_0 - %10 = OpExtInstWithForwardRefs %void %1 DebugTypeFunction %uint_0 %11 - %12 = OpExtInstWithForwardRefs %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 + %10 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeFunction %uint_0 %11 + %12 = OpExtInstWithForwardRefsKHR %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0 %11 = OpExtInst %void %1 DebugTypeComposite %3 %uint_0 %8 %uint_0 %uint_0 %9 %3 %uint_0 %uint_0 %12 %2 = OpFunction %void None %7 %13 = OpLabel