spirv-val: Add remaining OpSpecConstantOp (KhronosGroup#6596)

spencer-lunarg · web-flow · commit e4bceacf59fd · 2026-03-15T21:10:24.000-04:00
My personal final PR for KhronosGroup#6564 There is still the access chains left, but those are going to be ugly, and since its only a `Kernel` feature, I don't have the bandwidth to look into right now. (But did left some test for the person who does in the future)
diff --git a/source/val/validate_composites.cpp b/source/val/validate_composites.cpp
@@ -17,6 +17,7 @@
 // Validates correctness of composite SPIR-V instructions.
 
 #include <climits>
+#include <cstdint>
 
 #include "source/opcode.h"
 #include "source/spirv_target_env.h"
@@ -36,14 +37,11 @@ namespace {
 // deep).
 spv_result_t GetExtractInsertValueType(ValidationState_t& _,
                                        const Instruction* inst,
-                                       uint32_t* member_type) {
-  const spv::Op opcode = inst->opcode();
-  assert(opcode == spv::Op::OpCompositeExtract ||
-         opcode == spv::Op::OpCompositeInsert);
-  uint32_t word_index = opcode == spv::Op::OpCompositeExtract ? 4 : 5;
-  const uint32_t num_words = static_cast<uint32_t>(inst->words().size());
-  const uint32_t composite_id_index = word_index - 1;
-  const uint32_t num_indices = num_words - word_index;
+                                       uint32_t* member_type,
+                                       uint32_t composite_id_index) {
+  const uint32_t num_operands = static_cast<uint32_t>(inst->operands().size());
+  const uint32_t first_literal_index = composite_id_index + 1;
+  const uint32_t num_indices = num_operands - first_literal_index;
   const uint32_t kCompositeExtractInsertMaxNumIndices = 255;
 
   if (num_indices == 0) {
@@ -53,19 +51,21 @@ spv_result_t GetExtractInsertValueType(ValidationState_t& _,
 
   } else if (num_indices > kCompositeExtractInsertMaxNumIndices) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)
-           << "The number of indexes in Op" << spvOpcodeString(opcode)
+           << "The number of indexes in Op" << spvOpcodeString(inst->opcode())
            << " may not exceed " << kCompositeExtractInsertMaxNumIndices
            << ". Found " << num_indices << " indexes.";
   }
 
-  *member_type = _.GetTypeId(inst->word(composite_id_index));
+  *member_type = _.GetOperandTypeId(inst, composite_id_index);
   if (*member_type == 0) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)
            << "Expected Composite to be an object of composite type";
   }
 
-  for (; word_index < num_words; ++word_index) {
-    const uint32_t component_index = inst->word(word_index);
+  for (uint32_t operand_index = first_literal_index;
+       operand_index < num_operands; ++operand_index) {
+    const uint32_t component_index =
+        inst->GetOperandAs<uint32_t>(operand_index);
     const Instruction* const type_inst = _.FindDef(*member_type);
     assert(type_inst);
     switch (type_inst->opcode()) {
@@ -471,9 +471,12 @@ spv_result_t ValidateCompositeConstructReplicate(ValidationState_t& _,
 }
 
 spv_result_t ValidateCompositeExtract(ValidationState_t& _,
-                                      const Instruction* inst) {
+                                      const Instruction* inst,
+                                      uint32_t operand_index = 2) {
   uint32_t member_type = 0;
-  if (spv_result_t error = GetExtractInsertValueType(_, inst, &member_type)) {
+
+  if (spv_result_t error =
+          GetExtractInsertValueType(_, inst, &member_type, operand_index)) {
     return error;
   }
 
@@ -496,9 +499,10 @@ spv_result_t ValidateCompositeExtract(ValidationState_t& _,
 }
 
 spv_result_t ValidateCompositeInsert(ValidationState_t& _,
-                                     const Instruction* inst) {
-  const uint32_t object_type = _.GetOperandTypeId(inst, 2);
-  const uint32_t composite_type = _.GetOperandTypeId(inst, 3);
+                                     const Instruction* inst,
+                                     uint32_t operand_index = 2) {
+  const uint32_t object_type = _.GetOperandTypeId(inst, operand_index);
+  const uint32_t composite_type = _.GetOperandTypeId(inst, operand_index + 1);
   const uint32_t result_type = inst->type_id();
   if (result_type != composite_type) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)
@@ -508,7 +512,8 @@ spv_result_t ValidateCompositeInsert(ValidationState_t& _,
   }
 
   uint32_t member_type = 0;
-  if (spv_result_t error = GetExtractInsertValueType(_, inst, &member_type)) {
+  if (spv_result_t error =
+          GetExtractInsertValueType(_, inst, &member_type, operand_index + 1)) {
     return error;
   }
 
@@ -589,58 +594,58 @@ spv_result_t ValidateTranspose(ValidationState_t& _, const Instruction* inst) {
 }
 
 spv_result_t ValidateVectorShuffle(ValidationState_t& _,
-                                   const Instruction* inst) {
-  auto resultType = _.FindDef(inst->type_id());
-  if (!_.IsVectorType(resultType->id())) {
+                                   const Instruction* inst,
+                                   uint32_t operand_index = 2) {
+  auto result_type = _.FindDef(inst->type_id());
+  if (!_.IsVectorType(result_type->id())) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The Result Type of OpVectorShuffle must be"
            << " a vector type. Found Op"
-           << spvOpcodeString(resultType->opcode()) << ".";
+           << spvOpcodeString(result_type->opcode()) << ".";
   }
 
   // The number of components in Result Type must be the same as the number of
   // Component operands.
-  auto componentCount = inst->operands().size() - 4;
-  auto resultVectorDimension = _.GetDimension(resultType->id());
-  if (resultVectorDimension > 0 && componentCount != resultVectorDimension) {
+  uint32_t first_literal_index = operand_index + 2;
+  uint32_t component_count =
+      static_cast<uint32_t>(inst->operands().size()) - first_literal_index;
+  auto result_vec_dimension = _.GetDimension(result_type->id());
+  if (result_vec_dimension > 0 && component_count != result_vec_dimension) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "OpVectorShuffle component literals count does not match "
               "Result Type <id> "
-           << _.getIdName(resultType->id()) << "s vector component count.";
+           << _.getIdName(result_type->id()) << "s vector component count.";
   }
 
   // Vector 1 and Vector 2 must both have vector types, with the same Component
   // Type as Result Type.
-  auto vector1Object = _.FindDef(inst->GetOperandAs<uint32_t>(2));
-  auto vector1Type = _.FindDef(vector1Object->type_id());
-  auto vector2Object = _.FindDef(inst->GetOperandAs<uint32_t>(3));
-  auto vector2Type = _.FindDef(vector2Object->type_id());
-  if (!_.IsVectorType(vector1Type->id())) {
+  auto vec1_type = _.FindDef(_.GetOperandTypeId(inst, operand_index));
+  auto vec2_type = _.FindDef(_.GetOperandTypeId(inst, operand_index + 1));
+  if (!_.IsVectorType(vec1_type->id())) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The type of Vector 1 must be a vector type.";
   }
-  if (!_.IsVectorType(vector2Type->id())) {
+  if (!_.IsVectorType(vec2_type->id())) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The type of Vector 2 must be a vector type.";
   }
 
-  auto resultComponentType = resultType->GetOperandAs<uint32_t>(1);
-  if (vector1Type->GetOperandAs<uint32_t>(1) != resultComponentType) {
+  uint32_t result_component_type = result_type->GetOperandAs<uint32_t>(1);
+  if (vec1_type->GetOperandAs<uint32_t>(1) != result_component_type) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The Component Type of Vector 1 must be the same as ResultType.";
   }
-  if (vector2Type->GetOperandAs<uint32_t>(1) != resultComponentType) {
+  if (vec2_type->GetOperandAs<uint32_t>(1) != result_component_type) {
     return _.diag(SPV_ERROR_INVALID_ID, inst)
            << "The Component Type of Vector 2 must be the same as ResultType.";
   }
 
   // All Component literals must either be FFFFFFFF or in [0, N - 1].
-  auto vector1ComponentCount = vector1Type->GetOperandAs<uint32_t>(2);
-  auto vector2ComponentCount = vector2Type->GetOperandAs<uint32_t>(2);
-  auto N = vector1ComponentCount + vector2ComponentCount;
-  auto firstLiteralIndex = 4;
-  for (size_t i = firstLiteralIndex; i < inst->operands().size(); ++i) {
-    auto literal = inst->GetOperandAs<uint32_t>(i);
+  uint32_t vec1_component_count = vec1_type->GetOperandAs<uint32_t>(2);
+  uint32_t vec2_component_count = vec2_type->GetOperandAs<uint32_t>(2);
+  uint32_t N = vec1_component_count + vec2_component_count;
+  for (size_t i = first_literal_index; i < inst->operands().size(); ++i) {
+    uint32_t literal = inst->GetOperandAs<uint32_t>(i);
     if (literal != 0xFFFFFFFF && literal >= N) {
       return _.diag(SPV_ERROR_INVALID_ID, inst)
              << "Component index " << literal << " is out of bounds for "
@@ -1168,6 +1173,20 @@ spv_result_t CompositesPass(ValidationState_t& _, const Instruction* inst) {
       return ValidateCompositeExtractCoopMatQCOM(_, inst);
     case spv::Op::OpExtractSubArrayQCOM:
       return ValidateExtractSubArrayQCOM(_, inst);
+
+    case spv::Op::OpSpecConstantOp: {
+      switch (inst->GetOperandAs<spv::Op>(2u)) {
+        case spv::Op::OpVectorShuffle:
+          return ValidateVectorShuffle(_, inst, 3);
+        case spv::Op::OpCompositeExtract:
+          return ValidateCompositeExtract(_, inst, 3);
+        case spv::Op::OpCompositeInsert:
+          return ValidateCompositeInsert(_, inst, 3);
+        default:
+          break;
+      }
+    }
+
     default:
       break;
   }
diff --git a/source/val/validate_memory.cpp b/source/val/validate_memory.cpp
@@ -3535,13 +3535,14 @@ spv_result_t MemoryPass(ValidationState_t& _, const Instruction* inst) {
       return ValidatePtrComparison(_, inst);
     case spv::Op::OpImageTexelPointer:
     case spv::Op::OpGenericPtrMemSemantics:
-
+      break;  // no validation currently
     case spv::Op::OpSpecConstantOp: {
       switch (inst->GetOperandAs<spv::Op>(2u)) {
         case spv::Op::OpCooperativeMatrixLengthKHR:
           return ValidateCooperativeMatrixLength(_, inst, true, 3);
         case spv::Op::OpCooperativeMatrixLengthNV:
           return ValidateCooperativeMatrixLength(_, inst, false, 3);
+        // TODO - Add AccesChains
         default:
           break;
       }
diff --git a/test/val/val_constants_test.cpp b/test/val/val_constants_test.cpp
@@ -772,12 +772,16 @@ OpMemoryModel Logical VulkanKHR
               HasSubstr("must be OpTypeCooperativeMatrixKHR"));
 }
 
+// Some check use SPV_ERROR_INVALID_DATA vs SPV_ERROR_INVALID_ID
 #define BAD_KERNEL_OPERANDS(STR, ERR)                                   \
   {                                                                     \
     SPV_ENV_UNIVERSAL_1_0, kKernelPreamble kBasicTypes STR, false, ERR, \
         SPV_ERROR_INVALID_DATA                                          \
   }
 
+#define BAD_KERNEL_OPERANDS_ID(STR, ERR) \
+  { SPV_ENV_UNIVERSAL_1_0, kKernelPreamble kBasicTypes STR, false, ERR, }
+
 // 2 of each, first has bad return type, second has bad operand
 INSTANTIATE_TEST_SUITE_P(
     BadOperandsKernel, ValidateConstantOp,
@@ -1079,7 +1083,57 @@ INSTANTIATE_TEST_SUITE_P(
                             "float vector or scalar type"),
         BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %uint Bitcast %true",
                             "Expected input to be a pointer or int or float "
-                            "vector or scalar")}));
+                            "vector or scalar"),
+
+        BAD_KERNEL_OPERANDS_ID(
+            "%v = OpSpecConstantOp %float VectorShuffle %uint2_0 %uint2_0 1 3",
+            "The Result Type of OpVectorShuffle must be a vector type"),
+        BAD_KERNEL_OPERANDS_ID(
+            "%v = OpSpecConstantOp %uint2 VectorShuffle %uint2_0 %uint_0 1 3",
+            "The type of Vector 2 must be a vector type"),
+        BAD_KERNEL_OPERANDS(
+            "%v = OpSpecConstantOp %float CompositeExtract %uint2_0 1",
+            "Result type (OpTypeFloat) does not match the type that results "
+            "from indexing into the composite (OpTypeInt)"),
+        BAD_KERNEL_OPERANDS(
+            "%v = OpSpecConstantOp %uint CompositeExtract %uint_0 1",
+            "Reached non-composite type while indexes still remain to be "
+            "traversed"),
+        BAD_KERNEL_OPERANDS(
+            "%v = OpSpecConstantOp %float CompositeInsert %uint_0 %uint2_0 1",
+            "The Result Type must be the same as Composite type in "
+            "OpSpecConstantOp yielding Result Id 5"),
+        BAD_KERNEL_OPERANDS(
+            "%v = OpSpecConstantOp %uint2 CompositeInsert %uint_0 %uint_0 1",
+            "The Result Type must be the same as Composite type in "
+            "OpSpecConstantOp yielding Result Id 4"),
+
+        // TODO - Still need to add access chains
+        //
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %uint AccessChain %null",
+        //                     "AccessChain"),
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %_ptr_uint AccessChain
+        // %null %float_0",
+        //     "AccessChain"),
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %uint InBoundsAccessChain
+        // %null",
+        //     "InBoundsAccessChain"),
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %_ptr_uint
+        // InBoundsAccessChain %null %float_0",
+        //                     "InBoundsAccessChain"),
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %uint PtrAccessChain %null
+        // %uint_0",
+        //     "PtrAccessChain"),
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %_ptr_uint PtrAccessChain
+        // %float_0 %float_0",
+        //     "PtrAccessChain"),
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %uint
+        // InBoundsPtrAccessChain %null %uint_0",
+        //     "InBoundsPtrAccessChain"),
+        // BAD_KERNEL_OPERANDS("%v = OpSpecConstantOp %_ptr_uint
+        // InBoundsPtrAccessChain %float_0 %float_0",
+        //                     "InBoundsPtrAccessChain"),
+    }));
 
 }  // namespace
 }  // namespace val
