spirv-val, core: add support for OpExtInstWithForwardRefs (KhronosGro…

…up#5698) * val, core: add support for OpExtInstWithForwardRefs This commit adds validation and support for OpExtInstWithForwardRefs. This new instruction will be used for non-semantic debug info, when forward references are required. For now, this commit only fixes the code to handle this new instruction, and adds validation rules. But it does not add the pass to generate/fix the OpExtInst instruction when forward references are in use. Such pass would be useful for DXC or other tools, but I wanted to land validation rules first. This commit also bumps SPIRV-Headers to get this new opcode. --------- Signed-off-by: Nathan Gauër <[email protected]>
s-perron · Jun 4, 2024 · 6a2bdee · 6a2bdee
1 parent 4a2e0c9
commit 6a2bdee
Show file tree

Hide file tree

Showing 21 changed files with 293 additions and 28 deletions.
diff --git a/DEPS b/DEPS
@@ -14,7 +14,7 @@ vars = {
 
   're2_revision': '917047f3606d3ba9e2de0d383c3cd80c94ed732c',
 
-  'spirv_headers_revision': 'ea77f2a826bc820cb8f57f9b2a7c7eccb681c731',
+  'spirv_headers_revision': 'ff2afc3afc48dff4eec2a10f0212402a80708e38',
 }
 
 deps = {

diff --git a/source/binary.cpp b/source/binary.cpp
@@ -473,7 +473,7 @@ spv_result_t Parser::parseOperand(size_t inst_offset,
       if (!word) return diagnostic(SPV_ERROR_INVALID_ID) << "Id is 0";
       parsed_operand.type = SPV_OPERAND_TYPE_ID;
 
-      if (opcode == spv::Op::OpExtInst && parsed_operand.offset == 3) {
+      if (spvIsExtendedInstruction(opcode) && parsed_operand.offset == 3) {
         // The current word is the extended instruction set Id.
         // Set the extended instruction set type for the current instruction.
         auto ext_inst_type_iter = _.import_id_to_ext_inst_type.find(word);
@@ -494,7 +494,7 @@ spv_result_t Parser::parseOperand(size_t inst_offset,
       break;
 
     case SPV_OPERAND_TYPE_EXTENSION_INSTRUCTION_NUMBER: {
-      assert(spv::Op::OpExtInst == opcode);
+      assert(spvIsExtendedInstruction(opcode));
       assert(inst->ext_inst_type != SPV_EXT_INST_TYPE_NONE);
       spv_ext_inst_desc ext_inst;
       if (grammar_.lookupExtInst(inst->ext_inst_type, word, &ext_inst) ==

diff --git a/source/opcode.cpp b/source/opcode.cpp
@@ -720,6 +720,16 @@ bool spvOpcodeIsImageSample(const spv::Op opcode) {
   }
 }
 
+bool spvIsExtendedInstruction(const spv::Op opcode) {
+  switch (opcode) {
+    case spv::Op::OpExtInst:
+    case spv::Op::OpExtInstWithForwardRefs:
+      return true;
+    default:
+      return false;
+  }
+}
+
 std::vector<uint32_t> spvOpcodeMemorySemanticsOperandIndices(spv::Op opcode) {
   switch (opcode) {
     case spv::Op::OpMemoryBarrier:

diff --git a/source/opcode.h b/source/opcode.h
@@ -146,6 +146,9 @@ 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
+bool spvIsExtendedInstruction(spv::Op opcode);
+
 // Returns a vector containing the indices of the memory semantics <id>
 // operands for |opcode|.
 std::vector<uint32_t> spvOpcodeMemorySemanticsOperandIndices(spv::Op opcode);

diff --git a/source/operand.cpp b/source/operand.cpp
@@ -582,11 +582,13 @@ std::function<bool(unsigned)> spvOperandCanBeForwardDeclaredFunction(
 }
 
 std::function<bool(unsigned)> spvDbgInfoExtOperandCanBeForwardDeclaredFunction(
-    spv_ext_inst_type_t ext_type, uint32_t key) {
+    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 ever
+  // forward references except if used through OpExtInstWithForwardRefs.
   if (ext_type == SPV_EXT_INST_TYPE_NONSEMANTIC_SHADER_DEBUGINFO_100) {
-    return [](unsigned) { return false; };
+    return [opcode](unsigned) {
+      return opcode == spv::Op::OpExtInstWithForwardRefs;
+    };
   }
 
   // TODO(https://gitlab.khronos.org/spirv/SPIR-V/issues/532): Forward

diff --git a/source/operand.h b/source/operand.h
@@ -140,6 +140,6 @@ std::function<bool(unsigned)> spvOperandCanBeForwardDeclaredFunction(
 // of the operand can be forward declared. This function will
 // used in the SSA validation stage of the pipeline
 std::function<bool(unsigned)> spvDbgInfoExtOperandCanBeForwardDeclaredFunction(
-    spv_ext_inst_type_t ext_type, uint32_t key);
+    spv::Op opcode, spv_ext_inst_type_t ext_type, uint32_t key);
 
 #endif  // SOURCE_OPERAND_H_
diff --git a/source/opt/ir_context.h b/source/opt/ir_context.h
@@ -202,8 +202,8 @@ class IRContext {
   inline IteratorRange<Module::const_inst_iterator> debugs3() const;
 
   // Iterators for debug info instructions (excluding OpLine & OpNoLine)
-  // contained in this module.  These are OpExtInst for DebugInfo extension
-  // placed between section 9 and 10.
+  // contained in this module.  These are OpExtInst & OpExtInstWithForwardRefs
+  // 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<Module::inst_iterator> ext_inst_debuginfo();

diff --git a/source/opt/ir_loader.cpp b/source/opt/ir_loader.cpp
@@ -42,7 +42,7 @@ IrLoader::IrLoader(const MessageConsumer& consumer, Module* m)
 bool IsLineInst(const spv_parsed_instruction_t* inst) {
   const auto opcode = static_cast<spv::Op>(inst->opcode);
   if (IsOpLineInst(opcode)) return true;
-  if (opcode != spv::Op::OpExtInst) return false;
+  if (!spvIsExtendedInstruction(opcode)) return false;
   if (inst->ext_inst_type != SPV_EXT_INST_TYPE_NONSEMANTIC_SHADER_DEBUGINFO_100)
     return false;
   const uint32_t ext_inst_index = inst->words[kExtInstSetIndex];
@@ -65,7 +65,7 @@ bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
   // create a new instruction, but simply keep the information in
   // struct DebugScope.
   const auto opcode = static_cast<spv::Op>(inst->opcode);
-  if (opcode == spv::Op::OpExtInst &&
+  if (spvIsExtendedInstruction(opcode) &&
       spvExtInstIsDebugInfo(inst->ext_inst_type)) {
     const uint32_t ext_inst_index = inst->words[kExtInstSetIndex];
     if (inst->ext_inst_type == SPV_EXT_INST_TYPE_OPENCL_DEBUGINFO_100 ||
@@ -209,10 +209,10 @@ bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
       } else if (IsConstantInst(opcode) || opcode == spv::Op::OpVariable ||
                  opcode == spv::Op::OpUndef) {
         module_->AddGlobalValue(std::move(spv_inst));
-      } else if (opcode == spv::Op::OpExtInst &&
+      } else if (spvIsExtendedInstruction(opcode) &&
                  spvExtInstIsDebugInfo(inst->ext_inst_type)) {
         module_->AddExtInstDebugInfo(std::move(spv_inst));
-      } else if (opcode == spv::Op::OpExtInst &&
+      } else if (spvIsExtendedInstruction(opcode) &&
                  spvExtInstIsNonSemantic(inst->ext_inst_type)) {
         // If there are no functions, add the non-semantic instructions to the
         // global values. Otherwise append it to the list of the last function.
@@ -235,7 +235,7 @@ bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
         last_dbg_scope_ = DebugScope(kNoDebugScope, kNoInlinedAt);
       if (last_dbg_scope_.GetLexicalScope() != kNoDebugScope)
         spv_inst->SetDebugScope(last_dbg_scope_);
-      if (opcode == spv::Op::OpExtInst &&
+      if (spvIsExtendedInstruction(opcode) &&
           spvExtInstIsDebugInfo(inst->ext_inst_type)) {
         const uint32_t ext_inst_index = inst->words[kExtInstSetIndex];
         if (inst->ext_inst_type == SPV_EXT_INST_TYPE_OPENCL_DEBUGINFO_100) {

diff --git a/source/opt/strip_debug_info_pass.cpp b/source/opt/strip_debug_info_pass.cpp
@@ -43,7 +43,7 @@ Pass::Status StripDebugInfoPass::Process() {
           // see if this string is used anywhere by a non-semantic instruction
           bool no_nonsemantic_use =
               def_use->WhileEachUser(&inst, [def_use](Instruction* use) {
-                if (use->opcode() == spv::Op::OpExtInst) {
+                if (spvIsExtendedInstruction(use->opcode())) {
                   auto ext_inst_set =
                       def_use->GetDef(use->GetSingleWordInOperand(0u));
                   const std::string extension_name =

diff --git a/source/opt/strip_nonsemantic_info_pass.cpp b/source/opt/strip_nonsemantic_info_pass.cpp
@@ -96,7 +96,7 @@ Pass::Status StripNonSemanticInfoPass::Process() {
   if (!non_semantic_sets.empty()) {
     context()->module()->ForEachInst(
         [&non_semantic_sets, &to_remove](Instruction* inst) {
-          if (inst->opcode() == spv::Op::OpExtInst) {
+          if (spvIsExtendedInstruction(inst->opcode())) {
             if (non_semantic_sets.find(inst->GetSingleWordInOperand(0)) !=
                 non_semantic_sets.end()) {
               to_remove.push_back(inst);

diff --git a/source/text.cpp b/source/text.cpp
@@ -227,8 +227,7 @@ spv_result_t spvTextEncodeOperand(const spvtools::AssemblyGrammar& grammar,
 
       // Set the extended instruction type.
       // The import set id is the 3rd operand of OpExtInst.
-      if (spv::Op(pInst->opcode) == spv::Op::OpExtInst &&
-          pInst->words.size() == 4) {
+      if (spvIsExtendedInstruction(pInst->opcode) && pInst->words.size() == 4) {
         auto ext_inst_type = context->getExtInstTypeForId(pInst->words[3]);
         if (ext_inst_type == SPV_EXT_INST_TYPE_NONE) {
           return context->diagnostic()

diff --git a/source/val/instruction.h b/source/val/instruction.h
@@ -22,6 +22,7 @@
 #include <vector>
 
 #include "source/ext_inst.h"
+#include "source/opcode.h"
 #include "source/table.h"
 #include "spirv-tools/libspirv.h"
 
@@ -87,13 +88,13 @@ class Instruction {
   }
 
   bool IsNonSemantic() const {
-    return opcode() == spv::Op::OpExtInst &&
+    return spvIsExtendedInstruction(opcode()) &&
            spvExtInstIsNonSemantic(inst_.ext_inst_type);
   }
 
   /// True if this is an OpExtInst for debug info extension.
   bool IsDebugInfo() const {
-    return opcode() == spv::Op::OpExtInst &&
+    return spvIsExtendedInstruction(opcode()) &&
            spvExtInstIsDebugInfo(inst_.ext_inst_type);
   }
 

diff --git a/source/val/validate_adjacency.cpp b/source/val/validate_adjacency.cpp
@@ -52,6 +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:
         // 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
@@ -1684,6 +1684,7 @@ spv_result_t CheckIntegerWrapDecoration(ValidationState_t& vstate,
     case spv::Op::OpSNegate:
       return SPV_SUCCESS;
     case spv::Op::OpExtInst:
+    case spv::Op::OpExtInstWithForwardRefs:
       // TODO(dneto): Only certain extended instructions allow these
       // decorations.  For now allow anything.
       return SPV_SUCCESS;

diff --git a/source/val/validate_extensions.cpp b/source/val/validate_extensions.cpp
@@ -147,7 +147,7 @@ bool DoesDebugInfoOperandMatchExpectation(
     const Instruction* inst, uint32_t word_index) {
   if (inst->words().size() <= word_index) return false;
   auto* debug_inst = _.FindDef(inst->word(word_index));
-  if (debug_inst->opcode() != spv::Op::OpExtInst ||
+  if (!spvIsExtendedInstruction(debug_inst->opcode()) ||
       (debug_inst->ext_inst_type() != SPV_EXT_INST_TYPE_OPENCL_DEBUGINFO_100 &&
        debug_inst->ext_inst_type() !=
            SPV_EXT_INST_TYPE_NONSEMANTIC_SHADER_DEBUGINFO_100) ||
@@ -165,7 +165,7 @@ bool DoesDebugInfoOperandMatchExpectation(
     const Instruction* inst, uint32_t word_index) {
   if (inst->words().size() <= word_index) return false;
   auto* debug_inst = _.FindDef(inst->word(word_index));
-  if (debug_inst->opcode() != spv::Op::OpExtInst ||
+  if (!spvIsExtendedInstruction(debug_inst->opcode()) ||
       (debug_inst->ext_inst_type() !=
        SPV_EXT_INST_TYPE_NONSEMANTIC_SHADER_DEBUGINFO_100) ||
       !expectation(
@@ -409,7 +409,7 @@ spv_result_t ValidateClspvReflectionArgumentInfo(ValidationState_t& _,
 spv_result_t ValidateKernelDecl(ValidationState_t& _, const Instruction* inst) {
   const auto decl_id = inst->GetOperandAs<uint32_t>(4);
   const auto decl = _.FindDef(decl_id);
-  if (!decl || decl->opcode() != spv::Op::OpExtInst) {
+  if (!decl || !spvIsExtendedInstruction(decl->opcode())) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "Kernel must be a Kernel extended instruction";
   }
@@ -432,7 +432,7 @@ spv_result_t ValidateKernelDecl(ValidationState_t& _, const Instruction* inst) {
 spv_result_t ValidateArgInfo(ValidationState_t& _, const Instruction* inst,
                              uint32_t info_index) {
   auto info = _.FindDef(inst->GetOperandAs<uint32_t>(info_index));
-  if (!info || info->opcode() != spv::Op::OpExtInst) {
+  if (!info || !spvIsExtendedInstruction(info->opcode())) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "ArgInfo must be an ArgumentInfo extended instruction";
   }
@@ -3706,7 +3706,7 @@ spv_result_t ExtensionPass(ValidationState_t& _, const Instruction* inst) {
   const spv::Op opcode = inst->opcode();
   if (opcode == spv::Op::OpExtension) return ValidateExtension(_, inst);
   if (opcode == spv::Op::OpExtInstImport) return ValidateExtInstImport(_, inst);
-  if (opcode == spv::Op::OpExtInst) return ValidateExtInst(_, inst);
+  if (spvIsExtendedInstruction(opcode)) return ValidateExtInst(_, inst);
 
   return SPV_SUCCESS;
 }

diff --git a/source/val/validate_id.cpp b/source/val/validate_id.cpp
@@ -120,15 +120,16 @@ spv_result_t CheckIdDefinitionDominateUse(ValidationState_t& _) {
 // instruction operand's ID can be forward referenced.
 spv_result_t IdPass(ValidationState_t& _, Instruction* inst) {
   auto can_have_forward_declared_ids =
-      inst->opcode() == spv::Op::OpExtInst &&
+      spvIsExtendedInstruction(inst->opcode()) &&
               spvExtInstIsDebugInfo(inst->ext_inst_type())
           ? spvDbgInfoExtOperandCanBeForwardDeclaredFunction(
-                inst->ext_inst_type(), inst->word(4))
+                inst->opcode(), inst->ext_inst_type(), inst->word(4))
           : spvOperandCanBeForwardDeclaredFunction(inst->opcode());
 
   // Keep track of a result id defined by this instruction.  0 means it
   // does not define an id.
   uint32_t result_id = 0;
+  bool has_forward_declared_ids = false;
 
   for (unsigned i = 0; i < inst->operands().size(); i++) {
     const spv_parsed_operand_t& operand = inst->operand(i);
@@ -177,6 +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::OpExtInstImport &&
                      opcode != spv::Op::OpSelectionMerge &&
                      opcode != spv::Op::OpLoopMerge &&
@@ -200,6 +202,7 @@ spv_result_t IdPass(ValidationState_t& _, Instruction* inst) {
             ret = SPV_SUCCESS;
           }
         } else if (can_have_forward_declared_ids(i)) {
+          has_forward_declared_ids = true;
           if (spvOpcodeGeneratesType(inst->opcode()) &&
               !_.IsForwardPointer(operand_word)) {
             ret = _.diag(SPV_ERROR_INVALID_ID, inst)
@@ -229,12 +232,34 @@ spv_result_t IdPass(ValidationState_t& _, Instruction* inst) {
                 << " has not been defined";
         }
         break;
+      case SPV_OPERAND_TYPE_EXTENSION_INSTRUCTION_NUMBER:
+        // Ideally, this check would live in validate_extensions.cpp. But since
+        // forward references are only allowed on non-semantic instructions, and
+        // ID validation is done first, we would fail with a "ID had not been
+        // 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 &&
+            !inst->IsNonSemantic())
+          return _.diag(SPV_ERROR_INVALID_DATA, inst)
+                 << "OpExtInstWithForwardRefs is only allowed with "
+                    "non-semantic instructions.";
+        ret = SPV_SUCCESS;
+        break;
       default:
         ret = SPV_SUCCESS;
         break;
     }
     if (SPV_SUCCESS != ret) return ret;
   }
+  const bool must_have_forward_declared_ids =
+      inst->opcode() == spv::Op::OpExtInstWithForwardRefs;
+  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 "
+              "declared ID.";
+  }
+
   if (result_id) _.RemoveIfForwardDeclared(result_id);
 
   return SPV_SUCCESS;

diff --git a/source/val/validate_layout.cpp b/source/val/validate_layout.cpp
@@ -35,6 +35,7 @@ spv_result_t ModuleScopedInstructions(ValidationState_t& _,
                                       const Instruction* inst, spv::Op opcode) {
   switch (opcode) {
     case spv::Op::OpExtInst:
+    case spv::Op::OpExtInstWithForwardRefs:
       if (spvExtInstIsDebugInfo(inst->ext_inst_type())) {
         const uint32_t ext_inst_index = inst->word(4);
         bool local_debug_info = false;
@@ -243,6 +244,7 @@ spv_result_t FunctionScopedInstructions(ValidationState_t& _,
         break;
 
       case spv::Op::OpExtInst:
+      case spv::Op::OpExtInstWithForwardRefs:
         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
@@ -76,6 +76,7 @@ ModuleLayoutSection InstructionLayoutSection(
       if (current_section == kLayoutTypes) return kLayoutTypes;
       return kLayoutFunctionDefinitions;
     case spv::Op::OpExtInst:
+    case spv::Op::OpExtInstWithForwardRefs:
       // 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;