Update location/component conflict validation (#5993)
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
index 23e1857..06bae04 100644
--- a/source/val/validate_interfaces.cpp
+++ b/source/val/validate_interfaces.cpp
@@ -164,17 +164,22 @@
}
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 @@
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 @@
}
}
- // 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 @@
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))
+ return error;
+ uint32_t num_components = NumConsumedComponents(_, type);
+
+ 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 num_locations = 0;
- if (auto error = NumConsumedLocations(_, sub_type, &num_locations))
- return error;
- uint32_t num_components = NumConsumedComponents(_, sub_type);
+ if (kMaxLocations <= start) {
+ // Too many locations, give up.
+ return SPV_SUCCESS;
+ }
- 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;
- }
+ auto locs = locations;
+ if (has_index && index == 1) locs = output_index1_locations;
- 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;
-
- 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 @@
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
index f5ade33..bc9abf2 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 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
index c3a1c7c..2ab887f 100644
--- a/test/val/val_interfaces_test.cpp
+++ b/test/val/val_interfaces_test.cpp
@@ -1415,7 +1415,12 @@
)";
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 %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 @@
)";
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 @@
)";
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) {
