Update location/component conflict validation (#5993)

alan-baker · web-flow · commit ba1359d203e5 · 2025-03-14T22:43:00.000-04:00
Refs #5989 * Update input/output interface validation to treat arrays and matrices as consuming all 4 components in each location * For arrays and matrices make `NumConsumedComponents` more consistent * calculate per element components and round up to a multiple of 4
diff --git a/source/val/validate_interfaces.cpp b/source/val/validate_interfaces.cpp
@@ -164,17 +164,22 @@ spv_result_t NumConsumedLocations(ValidationState_t& _, const Instruction* type,
       }
       break;
     case spv::Op::OpTypeMatrix:
-      // Matrices consume locations equal to the underlying vector type for
-      // each column.
-      NumConsumedLocations(_, _.FindDef(type->GetOperandAs<uint32_t>(1)),
-                           num_locations);
+      // Matrices consume locations equivalent to arrays of 4-component vectors.
+      if (_.ContainsSizedIntOrFloatType(type->id(), spv::Op::OpTypeInt, 64) ||
+          _.ContainsSizedIntOrFloatType(type->id(), spv::Op::OpTypeFloat, 64)) {
+        *num_locations = 2;
+      } else {
+        *num_locations = 1;
+      }
       *num_locations *= type->GetOperandAs<uint32_t>(2);
       break;
     case spv::Op::OpTypeArray: {
       // Arrays consume locations equal to the underlying type times the number
       // of elements in the vector.
-      NumConsumedLocations(_, _.FindDef(type->GetOperandAs<uint32_t>(1)),
-                           num_locations);
+      if (auto error = NumConsumedLocations(
+              _, _.FindDef(type->GetOperandAs<uint32_t>(1)), num_locations)) {
+        return error;
+      }
       bool is_int = false;
       bool is_const = false;
       uint32_t value = 0;
@@ -244,10 +249,31 @@ uint32_t NumConsumedComponents(ValidationState_t& _, const Instruction* type) {
           NumConsumedComponents(_, _.FindDef(type->GetOperandAs<uint32_t>(1)));
       num_components *= type->GetOperandAs<uint32_t>(2);
       break;
-    case spv::Op::OpTypeArray:
-      // Skip the array.
-      return NumConsumedComponents(_,
-                                   _.FindDef(type->GetOperandAs<uint32_t>(1)));
+    case spv::Op::OpTypeMatrix:
+      // Matrices consume all components of the location.
+      // Round up to next multiple of 4.
+      num_components =
+          NumConsumedComponents(_, _.FindDef(type->GetOperandAs<uint32_t>(1)));
+      num_components *= type->GetOperandAs<uint32_t>(2);
+      num_components = ((num_components + 3) / 4) * 4;
+      break;
+    case spv::Op::OpTypeArray: {
+      // Arrays consume all components of the location.
+      // Round up to next multiple of 4.
+      num_components =
+          NumConsumedComponents(_, _.FindDef(type->GetOperandAs<uint32_t>(1)));
+
+      bool is_int = false;
+      bool is_const = false;
+      uint32_t value = 0;
+      // Attempt to evaluate the number of array elements.
+      std::tie(is_int, is_const, value) =
+          _.EvalInt32IfConst(type->GetOperandAs<uint32_t>(2));
+      if (is_int && is_const) num_components *= value;
+
+      num_components = ((num_components + 3) / 4) * 4;
+      return num_components;
+    }
     case spv::Op::OpTypePointer:
       if (_.addressing_model() ==
               spv::AddressingModel::PhysicalStorageBuffer64 &&
@@ -330,9 +356,10 @@ spv_result_t GetLocationsForVariable(
     }
   }
 
-  // Vulkan 14.1.3: Tessellation control and mesh per-vertex outputs and
-  // tessellation control, evaluation and geometry per-vertex inputs have a
-  // layer of arraying that is not included in interface matching.
+  // Vulkan 15.1.3 (Interface Matching): Tessellation control and mesh
+  // per-vertex outputs and tessellation control, evaluation and geometry
+  // per-vertex inputs have a layer of arraying that is not included in
+  // interface matching.
   bool is_arrayed = false;
   switch (entry_point->GetOperandAs<spv::ExecutionModel>(0)) {
     case spv::ExecutionModel::TessellationControl:
@@ -386,51 +413,33 @@ spv_result_t GetLocationsForVariable(
 
   const std::string storage_class = is_output ? "output" : "input";
   if (has_location) {
-    auto sub_type = type;
-    bool is_int = false;
-    bool is_const = false;
-    uint32_t array_size = 1;
-    // If the variable is still arrayed, mark the locations/components per
-    // index.
-    if (type->opcode() == spv::Op::OpTypeArray) {
-      // Determine the array size if possible and get the element type.
-      std::tie(is_int, is_const, array_size) =
-          _.EvalInt32IfConst(type->GetOperandAs<uint32_t>(2));
-      if (!is_int || !is_const) array_size = 1;
-      auto sub_type_id = type->GetOperandAs<uint32_t>(1);
-      sub_type = _.FindDef(sub_type_id);
-    }
-
     uint32_t num_locations = 0;
-    if (auto error = NumConsumedLocations(_, sub_type, &num_locations))
+    if (auto error = NumConsumedLocations(_, type, &num_locations))
       return error;
-    uint32_t num_components = NumConsumedComponents(_, sub_type);
+    uint32_t num_components = NumConsumedComponents(_, type);
 
-    for (uint32_t array_idx = 0; array_idx < array_size; ++array_idx) {
-      uint32_t array_location = location + (num_locations * array_idx);
-      uint32_t start = array_location * 4;
-      if (kMaxLocations <= start) {
-        // Too many locations, give up.
-        break;
-      }
+    uint32_t start = location * 4;
+    uint32_t end = (location + num_locations) * 4;
+    if (num_components % 4 != 0) {
+      start += component;
+      end = start + num_components;
+    }
 
-      uint32_t end = (array_location + num_locations) * 4;
-      if (num_components != 0) {
-        start += component;
-        end = array_location * 4 + component + num_components;
-      }
+    if (kMaxLocations <= start) {
+      // Too many locations, give up.
+      return SPV_SUCCESS;
+    }
 
-      auto locs = locations;
-      if (has_index && index == 1) locs = output_index1_locations;
+    auto locs = locations;
+    if (has_index && index == 1) locs = output_index1_locations;
 
-      for (uint32_t i = start; i < end; ++i) {
-        if (!locs->insert(i).second) {
-          return _.diag(SPV_ERROR_INVALID_DATA, entry_point)
-                 << (is_output ? _.VkErrorID(8722) : _.VkErrorID(8721))
-                 << "Entry-point has conflicting " << storage_class
-                 << " location assignment at location " << i / 4
-                 << ", component " << i % 4;
-        }
+    for (uint32_t i = start; i < end; ++i) {
+      if (!locs->insert(i).second) {
+        return _.diag(SPV_ERROR_INVALID_DATA, entry_point)
+               << (is_output ? _.VkErrorID(8722) : _.VkErrorID(8721))
+               << "Entry-point has conflicting " << storage_class
+               << " location assignment at location " << i / 4 << ", component "
+               << i % 4;
       }
     }
   } else {
@@ -489,38 +498,19 @@ spv_result_t GetLocationsForVariable(
         continue;
       }
 
-      if (member->opcode() == spv::Op::OpTypeArray && num_components >= 1 &&
-          num_components < 4) {
-        // When an array has an element that takes less than a location in
-        // size, calculate the used locations in a strided manner.
-        for (uint32_t l = location; l < num_locations + location; ++l) {
-          for (uint32_t c = component; c < component + num_components; ++c) {
-            uint32_t check = 4 * l + c;
-            if (!locations->insert(check).second) {
-              return _.diag(SPV_ERROR_INVALID_DATA, entry_point)
-                     << (is_output ? _.VkErrorID(8722) : _.VkErrorID(8721))
-                     << "Entry-point has conflicting " << storage_class
-                     << " location assignment at location " << l
-                     << ", component " << c;
-            }
-          }
-        }
-      } else {
-        // TODO: There is a hole here is the member is an array of 3- or
-        // 4-element vectors of 64-bit types.
-        uint32_t end = (location + num_locations) * 4;
-        if (num_components != 0) {
-          start += component;
-          end = location * 4 + component + num_components;
-        }
-        for (uint32_t l = start; l < end; ++l) {
-          if (!locations->insert(l).second) {
-            return _.diag(SPV_ERROR_INVALID_DATA, entry_point)
-                   << (is_output ? _.VkErrorID(8722) : _.VkErrorID(8721))
-                   << "Entry-point has conflicting " << storage_class
-                   << " location assignment at location " << l / 4
-                   << ", component " << l % 4;
-          }
+      uint32_t end = (location + num_locations) * 4;
+      if (num_components % 4 != 0) {
+        start += component;
+        end = location * 4 + component + num_components;
+      }
+
+      for (uint32_t l = start; l < end; ++l) {
+        if (!locations->insert(l).second) {
+          return _.diag(SPV_ERROR_INVALID_DATA, entry_point)
+                 << (is_output ? _.VkErrorID(8722) : _.VkErrorID(8721))
+                 << "Entry-point has conflicting " << storage_class
+                 << " location assignment at location " << l / 4
+                 << ", component " << l % 4;
         }
       }
     }
diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp
@@ -10394,7 +10394,7 @@ TEST_F(ValidateDecorations, ComponentMultipleArrays) {
                OpDecorate %gl_PerVertex Block
                OpDecorate %FOO Component 2
                OpDecorate %FOO Location 1
-               OpDecorate %FOO0 Location 1
+               OpDecorate %FOO0 Location 4
                OpDecorate %FOO0 Component 0
        %void = OpTypeVoid
           %3 = OpTypeFunction %void
diff --git a/test/val/val_interfaces_test.cpp b/test/val/val_interfaces_test.cpp
@@ -1415,7 +1415,12 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
-  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(getDiagnosticString(),
+              AnyVUID("VUID-StandaloneSpirv-OpEntryPoint-08722"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Entry-point has conflicting output location "
+                        "assignment at location 0, component 0"));
 }
 
 TEST_F(ValidateInterfacesTest, VulkanLocationsArrayWithComponentBad) {
@@ -1428,7 +1433,7 @@ OpDecorate %11 Location 0
 OpDecorate %18 Component 0
 OpDecorate %18 Location 0
 OpDecorate %28 Component 1
-OpDecorate %28 Location 0
+OpDecorate %28 Location 1
 OpDecorate %36 Location 1
 OpDecorate %40 Component 1
 OpDecorate %40 Location 1
@@ -1543,7 +1548,12 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
-  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(getDiagnosticString(),
+              AnyVUID("VUID-StandaloneSpirv-OpEntryPoint-08721"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Entry-point has conflicting input location "
+                        "assignment at location 0, component 0"));
 }
 
 TEST_F(ValidateInterfacesTest, VulkanLocationArrayWithComponent2) {
@@ -1575,7 +1585,12 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
-  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
+  EXPECT_THAT(getDiagnosticString(),
+              AnyVUID("VUID-StandaloneSpirv-OpEntryPoint-08721"));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Entry-point has conflicting input location "
+                        "assignment at location 0, component 0"));
 }
 
 TEST_F(ValidateInterfacesTest, DuplicateInterfaceVariableSuccess) {
