Revert array location validation (KhronosGroup#6187)

* Revert "Fix calculation of locations for matrices (KhronosGroup#6062)"

This reverts commit ada1771.

* Revert "Update location/component conflict validation (KhronosGroup#5993)"

This reverts commit ba1359d.

Author: alan-baker

source/val/validate_interfaces.cpp

@@ -166,20 +166,17 @@ spv_result_t NumConsumedLocations(ValidationState_t& _, const Instruction* type,
       }
       break;
     case spv::Op::OpTypeMatrix:
-      // Matrices consume locations equivalent to arrays.
-      if (auto error = NumConsumedLocations(
-              _, _.FindDef(type->GetOperandAs<uint32_t>(1)), num_locations)) {
-        return error;
-      }
+      // Matrices consume locations equal to the underlying vector type for
+      // each column.
+      NumConsumedLocations(_, _.FindDef(type->GetOperandAs<uint32_t>(1)),
+                           num_locations);
       *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.
-      if (auto error = NumConsumedLocations(
-              _, _.FindDef(type->GetOperandAs<uint32_t>(1)), num_locations)) {
-        return error;
-      }
+      NumConsumedLocations(_, _.FindDef(type->GetOperandAs<uint32_t>(1)),
+                           num_locations);
       bool is_int = false;
       bool is_const = false;
       uint32_t value = 0;
@@ -249,31 +246,10 @@ 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::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::OpTypeArray:
+      // Skip the array.
+      return NumConsumedComponents(_,
+                                   _.FindDef(type->GetOperandAs<uint32_t>(1)));
     case spv::Op::OpTypePointer:
       if (_.addressing_model() ==
               spv::AddressingModel::PhysicalStorageBuffer64 &&
@@ -356,10 +332,9 @@ spv_result_t GetLocationsForVariable(
     }
   }
 
-  // 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.
+  // 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.
   bool is_arrayed = false;
   switch (entry_point->GetOperandAs<spv::ExecutionModel>(0)) {
     case spv::ExecutionModel::TessellationControl:
@@ -413,33 +388,51 @@ 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(_, type, &num_locations))
+    if (auto error = NumConsumedLocations(_, sub_type, &num_locations))
       return error;
-    uint32_t num_components = NumConsumedComponents(_, type);
+    uint32_t num_components = NumConsumedComponents(_, sub_type);
 
-    uint32_t start = location * 4;
-    uint32_t end = (location + num_locations) * 4;
-    if (num_components % 4 != 0) {
-      start += component;
-      end = start + num_components;
-    }
+    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;
+      }
 
-    if (kMaxLocations <= start) {
-      // Too many locations, give up.
-      return SPV_SUCCESS;
-    }
+      uint32_t end = (array_location + num_locations) * 4;
+      if (num_components != 0) {
+        start += component;
+        end = array_location * 4 + component + num_components;
+      }
 
-    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 {
@@ -498,19 +491,38 @@ spv_result_t GetLocationsForVariable(
         continue;
       }
 
-      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;
+      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;
+          }
         }
       }
     }

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 4
+               OpDecorate %FOO0 Location 1
                OpDecorate %FOO0 Component 0
        %void = OpTypeVoid
           %3 = OpTypeFunction %void

test/val/val_interfaces_test.cpp

@@ -1415,12 +1415,7 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, 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"));
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
 }
 
 TEST_F(ValidateInterfacesTest, VulkanLocationsArrayWithComponentBad) {
@@ -1433,7 +1428,7 @@ OpDecorate %11 Location 0
 OpDecorate %18 Component 0
 OpDecorate %18 Location 0
 OpDecorate %28 Component 1
-OpDecorate %28 Location 1
+OpDecorate %28 Location 0
 OpDecorate %36 Location 1
 OpDecorate %40 Component 1
 OpDecorate %40 Location 1
@@ -1548,12 +1543,7 @@ OpFunctionEnd
 )";
 
   CompileSuccessfully(text, 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"));
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
 }
 
 TEST_F(ValidateInterfacesTest, VulkanLocationArrayWithComponent2) {
@@ -1582,85 +1572,12 @@ OpMemberDecorate %struct 1 Component 2
 %entry = OpLabel
 OpReturn
 OpFunctionEnd
-)";
-
-  CompileSuccessfully(text, 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, VulkanLocationMatrix2x2DoubleGood) {
-  const std::string text = R"(
-OpCapability Shader
-OpCapability Float64
-OpMemoryModel Logical GLSL450
-OpEntryPoint Fragment %main "main" %in
-OpExecutionMode %main OriginUpperLeft
-OpDecorate %struct Block
-OpMemberDecorate %struct 0 Location 0
-OpMemberDecorate %struct 1 Location 2
-%void = OpTypeVoid
-%void_fn = OpTypeFunction %void
-%float = OpTypeFloat 32
-%double = OpTypeFloat 64
-%int = OpTypeInt 32 0
-%int_2 = OpConstant %int 2
-%double2 = OpTypeVector %double 2
-%mat2x2_double = OpTypeMatrix %double2 2
-%struct = OpTypeStruct %mat2x2_double %int
-%ptr = OpTypePointer Input %struct
-%in = OpVariable %ptr Input
-%main = OpFunction %void None %void_fn
-%entry = OpLabel
-OpReturn
-OpFunctionEnd
 )";
 
   CompileSuccessfully(text, SPV_ENV_VULKAN_1_0);
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0));
 }
 
-TEST_F(ValidateInterfacesTest, VulkanLocationMatrix2x2DoubleBad) {
-  const std::string text = R"(
-OpCapability Shader
-OpCapability Float64
-OpMemoryModel Logical GLSL450
-OpEntryPoint Fragment %main "main" %in
-OpExecutionMode %main OriginUpperLeft
-OpDecorate %struct Block
-OpMemberDecorate %struct 0 Location 0
-OpMemberDecorate %struct 1 Location 1
-OpMemberDecorate %struct 1 Component 3
-%void = OpTypeVoid
-%void_fn = OpTypeFunction %void
-%float = OpTypeFloat 32
-%double = OpTypeFloat 64
-%int = OpTypeInt 32 0
-%int_2 = OpConstant %int 2
-%double2 = OpTypeVector %double 2
-%mat2x2_double = OpTypeMatrix %double2 2
-%struct = OpTypeStruct %mat2x2_double %int
-%ptr = OpTypePointer Input %struct
-%in = OpVariable %ptr Input
-%main = OpFunction %void None %void_fn
-%entry = OpLabel
-OpReturn
-OpFunctionEnd
-)";
-
-  CompileSuccessfully(text, 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 1, component 3"));
-}
-
 TEST_F(ValidateInterfacesTest, DuplicateInterfaceVariableSuccess) {
   const std::string text = R"(
 OpCapability Shader