Validate duplicate decorations and execution modes (KhronosGroup#5641)

alan-baker · web-flow · commit 02470f606fe1 · 2024-04-12T08:51:41.000-04:00
* Disallow duplicate decorations generally

* Only FuncParamAttr and UserSemantic can be applied to the same target
  multiple times
* Unchecked: completely duplicate UserSemantic and FuncParamAttr

* Disallow duplicate execution modes generally
  * Exceptions for float controls, float controls2 and some intel
    execution modes

* Fix invalid fuzzer transforms
diff --git a/source/fuzz/transformation_add_no_contraction_decoration.cpp b/source/fuzz/transformation_add_no_contraction_decoration.cpp
@@ -36,6 +36,11 @@ bool TransformationAddNoContractionDecoration::IsApplicable(
   if (!instr) {
     return false;
   }
+  // |instr| must not be decorated with NoContraction.
+  if (ir_context->get_decoration_mgr()->HasDecoration(
+          message_.result_id(), spv::Decoration::NoContraction)) {
+    return false;
+  }
   // The instruction must be arithmetic.
   return IsArithmetic(instr->opcode());
 }
diff --git a/source/fuzz/transformation_add_relaxed_decoration.cpp b/source/fuzz/transformation_add_relaxed_decoration.cpp
@@ -36,6 +36,11 @@ bool TransformationAddRelaxedDecoration::IsApplicable(
   if (!instr) {
     return false;
   }
+  // |instr| must not be decorated with RelaxedPrecision.
+  if (ir_context->get_decoration_mgr()->HasDecoration(
+          message_.result_id(), spv::Decoration::RelaxedPrecision)) {
+    return false;
+  }
   opt::BasicBlock* cur_block = ir_context->get_instr_block(instr);
   // The instruction must have a block.
   if (cur_block == nullptr) {
@@ -46,6 +51,7 @@ bool TransformationAddRelaxedDecoration::IsApplicable(
           cur_block->id()))) {
     return false;
   }
+
   // The instruction must be numeric.
   return IsNumeric(instr->opcode());
 }
diff --git a/source/val/validate.cpp b/source/val/validate.cpp
@@ -144,6 +144,9 @@ spv_result_t ValidateEntryPoints(ValidationState_t& _) {
   if (auto error = ValidateFloatControls2(_)) {
     return error;
   }
+  if (auto error = ValidateDuplicateExecutionModes(_)) {
+    return error;
+  }
 
   return SPV_SUCCESS;
 }
diff --git a/source/val/validate.h b/source/val/validate.h
@@ -94,6 +94,13 @@ spv_result_t ValidateInterfaces(ValidationState_t& _);
 /// @return SPV_SUCCESS if no errors are found.
 spv_result_t ValidateFloatControls2(ValidationState_t& _);
 
+/// @brief Validates duplicated execution modes for each entry point.
+///
+/// @param[in] _ the validation state of the module
+///
+/// @return SPV_SUCCESS if no errors are found.
+spv_result_t ValidateDuplicateExecutionModes(ValidationState_t& _);
+
 /// @brief Validates memory instructions
 ///
 /// @param[in] _ the validation state of the module
diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp
@@ -1325,21 +1325,14 @@ spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) {
 
 // Returns true if |decoration| cannot be applied to the same id more than once.
 bool AtMostOncePerId(spv::Decoration decoration) {
-  return decoration == spv::Decoration::ArrayStride;
+  return decoration != spv::Decoration::UserSemantic &&
+         decoration != spv::Decoration::FuncParamAttr;
 }
 
 // Returns true if |decoration| cannot be applied to the same member more than
 // once.
 bool AtMostOncePerMember(spv::Decoration decoration) {
-  switch (decoration) {
-    case spv::Decoration::Offset:
-    case spv::Decoration::MatrixStride:
-    case spv::Decoration::RowMajor:
-    case spv::Decoration::ColMajor:
-      return true;
-    default:
-      return false;
-  }
+  return decoration != spv::Decoration::UserSemantic;
 }
 
 spv_result_t CheckDecorationsCompatibility(ValidationState_t& vstate) {
diff --git a/source/val/validate_interfaces.cpp b/source/val/validate_interfaces.cpp
@@ -254,37 +254,24 @@ spv_result_t GetLocationsForVariable(
   // equal. Also track Patch and PerTaskNV decorations.
   bool has_location = false;
   uint32_t location = 0;
-  bool has_component = false;
   uint32_t component = 0;
   bool has_index = false;
   uint32_t index = 0;
   bool has_patch = false;
   bool has_per_task_nv = false;
   bool has_per_vertex_khr = false;
+  // Duplicate Location, Component, Index are checked elsewhere.
   for (auto& dec : _.id_decorations(variable->id())) {
     if (dec.dec_type() == spv::Decoration::Location) {
-      if (has_location && dec.params()[0] != location) {
-        return _.diag(SPV_ERROR_INVALID_DATA, variable)
-               << "Variable has conflicting location decorations";
-      }
       has_location = true;
       location = dec.params()[0];
     } else if (dec.dec_type() == spv::Decoration::Component) {
-      if (has_component && dec.params()[0] != component) {
-        return _.diag(SPV_ERROR_INVALID_DATA, variable)
-               << "Variable has conflicting component decorations";
-      }
-      has_component = true;
       component = dec.params()[0];
     } else if (dec.dec_type() == spv::Decoration::Index) {
       if (!is_output || !is_fragment) {
         return _.diag(SPV_ERROR_INVALID_DATA, variable)
                << "Index can only be applied to Fragment output variables";
       }
-      if (has_index && dec.params()[0] != index) {
-        return _.diag(SPV_ERROR_INVALID_DATA, variable)
-               << "Variable has conflicting index decorations";
-      }
       has_index = true;
       index = dec.params()[0];
     } else if (dec.dec_type() == spv::Decoration::BuiltIn) {
diff --git a/source/val/validate_mode_setting.cpp b/source/val/validate_mode_setting.cpp
@@ -724,6 +724,25 @@ spv_result_t ValidateMemoryModel(ValidationState_t& _,
   return SPV_SUCCESS;
 }
 
+bool PerEntryExecutionMode(spv::ExecutionMode mode) {
+  switch (mode) {
+    // These execution modes can be specified multiple times per entry point.
+    case spv::ExecutionMode::DenormPreserve:
+    case spv::ExecutionMode::DenormFlushToZero:
+    case spv::ExecutionMode::SignedZeroInfNanPreserve:
+    case spv::ExecutionMode::RoundingModeRTE:
+    case spv::ExecutionMode::RoundingModeRTZ:
+    case spv::ExecutionMode::FPFastMathDefault:
+    case spv::ExecutionMode::RoundingModeRTPINTEL:
+    case spv::ExecutionMode::RoundingModeRTNINTEL:
+    case spv::ExecutionMode::FloatingPointModeALTINTEL:
+    case spv::ExecutionMode::FloatingPointModeIEEEINTEL:
+      return false;
+    default:
+      return true;
+  }
+}
+
 }  // namespace
 
 spv_result_t ValidateFloatControls2(ValidationState_t& _) {
@@ -808,5 +827,52 @@ spv_result_t ModeSettingPass(ValidationState_t& _, const Instruction* inst) {
   return SPV_SUCCESS;
 }
 
+spv_result_t ValidateDuplicateExecutionModes(ValidationState_t& _) {
+  using PerEntryKey = std::tuple<spv::ExecutionMode, uint32_t>;
+  using PerOperandKey = std::tuple<spv::ExecutionMode, uint32_t, uint32_t>;
+  std::set<PerEntryKey> seen_per_entry;
+  std::set<PerOperandKey> seen_per_operand;
+
+  const auto lookupMode = [&_](spv::ExecutionMode mode) -> std::string {
+    spv_operand_desc desc = nullptr;
+    if (_.grammar().lookupOperand(SPV_OPERAND_TYPE_EXECUTION_MODE,
+                                  static_cast<uint32_t>(mode),
+                                  &desc) == SPV_SUCCESS) {
+      return std::string(desc->name);
+    }
+    return "Unknown";
+  };
+
+  for (const auto& inst : _.ordered_instructions()) {
+    if (inst.opcode() != spv::Op::OpExecutionMode &&
+        inst.opcode() != spv::Op::OpExecutionModeId) {
+      continue;
+    }
+
+    const auto entry = inst.GetOperandAs<uint32_t>(0);
+    const auto mode = inst.GetOperandAs<spv::ExecutionMode>(1);
+    if (PerEntryExecutionMode(mode)) {
+      if (!seen_per_entry.insert(std::make_tuple(mode, entry)).second) {
+        return _.diag(SPV_ERROR_INVALID_ID, &inst)
+               << lookupMode(mode)
+               << " execution mode must not be specified multiple times per "
+                  "entry point";
+      }
+    } else {
+      // Execution modes allowed multiple times all take a single operand.
+      const auto operand = inst.GetOperandAs<uint32_t>(2);
+      if (!seen_per_operand.insert(std::make_tuple(mode, entry, operand))
+               .second) {
+        return _.diag(SPV_ERROR_INVALID_ID, &inst)
+               << lookupMode(mode)
+               << " execution mode must not be specified multiple times for "
+                  "the same entry point and operands";
+      }
+    }
+  }
+
+  return SPV_SUCCESS;
+}
+
 }  // namespace val
 }  // namespace spvtools
diff --git a/test/fuzz/transformation_add_no_contraction_decoration_test.cpp b/test/fuzz/transformation_add_no_contraction_decoration_test.cpp
@@ -36,7 +36,6 @@ TEST(TransformationAddNoContractionDecorationTest, BasicScenarios) {
                OpName %8 "x"
                OpName %10 "y"
                OpName %14 "i"
-               OpDecorate %32 NoContraction
           %2 = OpTypeVoid
           %3 = OpTypeFunction %2
           %6 = OpTypeFloat 32
@@ -110,9 +109,8 @@ TEST(TransformationAddNoContractionDecorationTest, BasicScenarios) {
   ASSERT_FALSE(TransformationAddNoContractionDecoration(24).IsApplicable(
       context.get(), transformation_context));
 
-  // It is valid to add NoContraction to each of these ids (and it's fine to
-  // have duplicates of the decoration, in the case of 32).
-  for (uint32_t result_id : {32u, 32u, 27u, 29u, 39u}) {
+  // It is valid to add NoContraction to each of these ids.
+  for (uint32_t result_id : {32u, 27u, 29u, 39u}) {
     TransformationAddNoContractionDecoration transformation(result_id);
     ASSERT_TRUE(
         transformation.IsApplicable(context.get(), transformation_context));
@@ -134,8 +132,6 @@ TEST(TransformationAddNoContractionDecorationTest, BasicScenarios) {
                OpName %10 "y"
                OpName %14 "i"
                OpDecorate %32 NoContraction
-               OpDecorate %32 NoContraction
-               OpDecorate %32 NoContraction
                OpDecorate %27 NoContraction
                OpDecorate %29 NoContraction
                OpDecorate %39 NoContraction
diff --git a/test/fuzz/transformation_add_relaxed_decoration_test.cpp b/test/fuzz/transformation_add_relaxed_decoration_test.cpp
@@ -86,9 +86,8 @@ TEST(TransformationAddRelaxedDecorationTest, BasicScenarios) {
   // Invalid: 28 is in a dead block, but returns bool (not numeric).
   ASSERT_FALSE(TransformationAddRelaxedDecoration(28).IsApplicable(
       context.get(), transformation_context));
-  // It is valid to add RelaxedPrecision to 25 (and it's fine to
-  // have a duplicate).
-  for (uint32_t result_id : {25u, 25u}) {
+  // It is valid to add RelaxedPrecision to 25
+  for (uint32_t result_id : {25u}) {
     TransformationAddRelaxedDecoration transformation(result_id);
     ASSERT_TRUE(
         transformation.IsApplicable(context.get(), transformation_context));
@@ -110,7 +109,6 @@ TEST(TransformationAddRelaxedDecorationTest, BasicScenarios) {
                OpName %10 "b"
                OpName %14 "c"
                OpDecorate %25 RelaxedPrecision
-               OpDecorate %25 RelaxedPrecision
           %2 = OpTypeVoid
           %3 = OpTypeFunction %2
           %6 = OpTypeInt 32 1
diff --git a/test/val/val_interfaces_test.cpp b/test/val/val_interfaces_test.cpp
@@ -419,9 +419,10 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
-  EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("Variable has conflicting location decorations"));
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("decorated with Location multiple times is not allowed"));
 }
 
 TEST_F(ValidateInterfacesTest, VulkanLocationsVariableAndMemberAssigned) {
@@ -505,9 +506,10 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
-  EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("Member index 1 has conflicting location assignments"));
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("decorated with Location multiple times is not allowed"));
 }
 
 TEST_F(ValidateInterfacesTest, VulkanLocationsMissingAssignmentStructMember) {
@@ -1157,9 +1159,10 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
-  EXPECT_THAT(getDiagnosticString(),
-              HasSubstr("Variable has conflicting component decorations"));
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("decorated with Component multiple times is not allowed"));
 }
 
 TEST_F(ValidateInterfacesTest,
@@ -1186,10 +1189,10 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
   EXPECT_THAT(
       getDiagnosticString(),
-      HasSubstr("Member index 0 has conflicting component assignments"));
+      HasSubstr("decorated with Component multiple times is not allowed"));
 }
 
 TEST_F(ValidateInterfacesTest, VulkanLocationsVariableConflictOutputIndex1) {
diff --git a/test/val/val_modes_test.cpp b/test/val/val_modes_test.cpp

Original file line number	Diff line number	Diff line change
`@@ -36,6 +36,11 @@ bool TransformationAddNoContractionDecoration::IsApplicable(`
`36`	`36`	`if (!instr) {`
`37`	`37`	`return false;`
`38`	`38`	`}`
	`39`	`+ // \|instr\| must not be decorated with NoContraction.`
	`40`	`+ if (ir_context->get_decoration_mgr()->HasDecoration(`
	`41`	`+ message_.result_id(), spv::Decoration::NoContraction)) {`
	`42`	`+ return false;`
	`43`	`+ }`
`39`	`44`	`// The instruction must be arithmetic.`
`40`	`45`	`return IsArithmetic(instr->opcode());`
`41`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -144,6 +144,9 @@ spv_result_t ValidateEntryPoints(ValidationState_t& _) {`
`144`	`144`	`if (auto error = ValidateFloatControls2(_)) {`
`145`	`145`	`return error;`
`146`	`146`	`}`
	`147`	`+ if (auto error = ValidateDuplicateExecutionModes(_)) {`
	`148`	`+ return error;`
	`149`	`+ }`
`147`	`150`
`148`	`151`	`return SPV_SUCCESS;`
`149`	`152`	`}`