Revert array location validation (#6187)
* Revert "Fix calculation of locations for matrices (#6062)"
This reverts commit ada1771a9f7a125573aa94fe551fdc44b45769bd.
* Revert "Update location/component conflict validation (#5993)"
This reverts commit ba1359d203e544bd458373ba249a47c44e97b071.
diff --git a/source/val/validate_interfaces.cpp b/source/val/validate_interfaces.cpp
index fcb5e32..b80c7d8 100644
--- a/source/val/validate_interfaces.cpp
+++ b/source/val/validate_interfaces.cpp
@@ -166,20 +166,17 @@
}
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 @@
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 @@
}
}
- // 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 @@
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 @@
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;
+ }
}
}
}
diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp
index 3500414..087d416 100644
--- a/test/val/val_decoration_test.cpp
+++ b/test/val/val_decoration_test.cpp
@@ -10394,7 +10394,7 @@
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
diff --git a/test/val/val_interfaces_test.cpp b/test/val/val_interfaces_test.cpp
index f12ae72..e5cc97d 100644
--- a/test/val/val_interfaces_test.cpp
+++ b/test/val/val_interfaces_test.cpp
@@ -1415,12 +1415,7 @@
)";
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 %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 @@
)";
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) {
@@ -1585,82 +1575,9 @@
)";
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
