spirv-reader: Ignore duplicate decorations
Vulkan allows them, but WGSL does not.
A duplicate decoration is purely redundant.
A parameterized decoratio with different parameterization is
an inconsistency and a semantic error, at least for currently defined
SPIR-V decorations.
So for each target, only take the first decoration of each kind.
Fixed: tint:1337
Change-Id: I6ed5c39cf2e213c695cb8217ed1b97814da3db56
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/72500
Reviewed-by: Ben Clayton <bclayton@google.com>
Auto-Submit: David Neto <dneto@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: James Price <jrprice@google.com>
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index 544fcfe..4f28c19 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/src/reader/spirv/parser_impl.cc
@@ -388,16 +388,20 @@
DecorationList ParserImpl::GetDecorationsFor(uint32_t id) const {
DecorationList result;
const auto& decorations = deco_mgr_->GetDecorationsFor(id, true);
+ std::unordered_set<uint32_t> visited;
for (const auto* inst : decorations) {
if (inst->opcode() != SpvOpDecorate) {
continue;
}
// Example: OpDecorate %struct_id Block
// Example: OpDecorate %array_ty ArrayStride 16
- std::vector<uint32_t> inst_as_words;
- inst->ToBinaryWithoutAttachedDebugInsts(&inst_as_words);
- Decoration d(inst_as_words.begin() + 2, inst_as_words.end());
- result.push_back(d);
+ auto decoration_kind = inst->GetSingleWordInOperand(1);
+ if (visited.emplace(decoration_kind).second) {
+ std::vector<uint32_t> inst_as_words;
+ inst->ToBinaryWithoutAttachedDebugInsts(&inst_as_words);
+ Decoration d(inst_as_words.begin() + 2, inst_as_words.end());
+ result.push_back(d);
+ }
}
return result;
}
@@ -407,16 +411,20 @@
uint32_t member_index) const {
DecorationList result;
const auto& decorations = deco_mgr_->GetDecorationsFor(id, true);
+ std::unordered_set<uint32_t> visited;
for (const auto* inst : decorations) {
+ // Example: OpMemberDecorate %struct_id 1 Offset 16
if ((inst->opcode() != SpvOpMemberDecorate) ||
(inst->GetSingleWordInOperand(1) != member_index)) {
continue;
}
- // Example: OpMemberDecorate %struct_id 2 Offset 24
- std::vector<uint32_t> inst_as_words;
- inst->ToBinaryWithoutAttachedDebugInsts(&inst_as_words);
- Decoration d(inst_as_words.begin() + 3, inst_as_words.end());
- result.push_back(d);
+ auto decoration_kind = inst->GetSingleWordInOperand(2);
+ if (visited.emplace(decoration_kind).second) {
+ std::vector<uint32_t> inst_as_words;
+ inst->ToBinaryWithoutAttachedDebugInsts(&inst_as_words);
+ Decoration d(inst_as_words.begin() + 3, inst_as_words.end());
+ result.push_back(d);
+ }
}
return result;
}
@@ -1046,7 +1054,6 @@
bool ParserImpl::ParseArrayDecorations(
const spvtools::opt::analysis::Type* spv_type,
uint32_t* array_stride) {
- bool has_array_stride = false;
*array_stride = 0; // Implicit stride case.
const auto type_id = type_mgr_->GetId(spv_type);
for (auto& decoration : this->GetDecorationsFor(type_id)) {
@@ -1056,11 +1063,6 @@
return Fail() << "invalid array type ID " << type_id
<< ": ArrayStride can't be 0";
}
- if (has_array_stride) {
- return Fail() << "invalid array type ID " << type_id
- << ": multiple ArrayStride decorations";
- }
- has_array_stride = true;
*array_stride = stride;
} else {
return Fail() << "invalid array type ID " << type_id
diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h
index 9895439..de9f71a 100644
--- a/src/reader/spirv/parser_impl.h
+++ b/src/reader/spirv/parser_impl.h
@@ -218,16 +218,16 @@
/// This is null until BuildInternalModule has been called.
spvtools::opt::IRContext* ir_context() { return ir_context_.get(); }
- /// Gets the list of decorations for a SPIR-V result ID. Returns an empty
- /// vector if the ID is not a result ID, or if no decorations target that ID.
- /// The internal representation must have already been built.
+ /// Gets the list of unique decorations for a SPIR-V result ID. Returns an
+ /// empty vector if the ID is not a result ID, or if no decorations target
+ /// that ID. The internal representation must have already been built.
/// @param id SPIR-V ID
/// @returns the list of decorations on the given ID
DecorationList GetDecorationsFor(uint32_t id) const;
- /// Gets the list of decorations for the member of a struct. Returns an empty
- /// list if the `id` is not the ID of a struct, or if the member index is out
- /// of range, or if the target member has no decorations.
- /// The internal representation must have already been built.
+ /// Gets the list of unique decorations for the member of a struct. Returns
+ /// an empty list if the `id` is not the ID of a struct, or if the member
+ /// index is out of range, or if the target member has no decorations. The
+ /// internal representation must have already been built.
/// @param id SPIR-V ID of a struct
/// @param member_index the member within the struct
/// @returns the list of decorations on the member
diff --git a/src/reader/spirv/parser_impl_convert_type_test.cc b/src/reader/spirv/parser_impl_convert_type_test.cc
index 1828362..8ad4177 100644
--- a/src/reader/spirv/parser_impl_convert_type_test.cc
+++ b/src/reader/spirv/parser_impl_convert_type_test.cc
@@ -401,8 +401,8 @@
auto* type = p->ConvertType(10);
ASSERT_NE(type, nullptr);
auto* arr_type = type->UnwrapAll()->As<Array>();
- EXPECT_EQ(arr_type->size, 0u);
ASSERT_NE(arr_type, nullptr);
+ EXPECT_EQ(arr_type->size, 0u);
EXPECT_EQ(arr_type->stride, 64u);
}
@@ -420,18 +420,22 @@
}
TEST_F(SpvParserTest,
- ConvertType_RuntimeArray_ArrayStride_SpecifiedTwiceIsError) {
+ ConvertType_RuntimeArray_ArrayStride_SpecifiedTwiceTakeTheFirstStride) {
+ // This is an inconsistent input module. Be resilient and
+ // take only the first stride.
auto p = parser(test::Assemble(Preamble() + R"(
OpDecorate %10 ArrayStride 64
- OpDecorate %10 ArrayStride 64
+ OpDecorate %10 ArrayStride 32
%uint = OpTypeInt 32 0
%10 = OpTypeRuntimeArray %uint
)" + MainBody()));
EXPECT_TRUE(p->BuildInternalModule());
auto* type = p->ConvertType(10);
- EXPECT_EQ(type, nullptr);
- EXPECT_THAT(p->error(),
- Eq("invalid array type ID 10: multiple ArrayStride decorations"));
+ ASSERT_NE(type, nullptr);
+ auto* arr_type = type->UnwrapAll()->As<Array>();
+ ASSERT_NE(arr_type, nullptr);
+ EXPECT_EQ(arr_type->size, 0u);
+ EXPECT_EQ(arr_type->stride, 64u);
}
TEST_F(SpvParserTest, ConvertType_Array) {
@@ -552,10 +556,13 @@
Eq("invalid array type ID 10: ArrayStride can't be 0"));
}
-TEST_F(SpvParserTest, ConvertType_ArrayStride_SpecifiedTwiceIsError) {
+TEST_F(SpvParserTest,
+ ConvertType_ArrayStride_SpecifiedTwiceTakeTheFirstStride) {
+ // This is an inconsistent input module. Be resilient and
+ // take only the first stride.
auto p = parser(test::Assemble(Preamble() + R"(
OpDecorate %10 ArrayStride 4
- OpDecorate %10 ArrayStride 4
+ OpDecorate %10 ArrayStride 8
%uint = OpTypeInt 32 0
%uint_5 = OpConstant %uint 5
%10 = OpTypeArray %uint %uint_5
@@ -563,9 +570,12 @@
EXPECT_TRUE(p->BuildInternalModule());
auto* type = p->ConvertType(10);
- ASSERT_EQ(type, nullptr);
- EXPECT_THAT(p->error(),
- Eq("invalid array type ID 10: multiple ArrayStride decorations"));
+ ASSERT_NE(type, nullptr);
+ EXPECT_TRUE(type->UnwrapAll()->Is<Array>());
+ auto* arr_type = type->UnwrapAll()->As<Array>();
+ ASSERT_NE(arr_type, nullptr);
+ EXPECT_EQ(arr_type->stride, 4u);
+ EXPECT_TRUE(p->error().empty());
}
TEST_F(SpvParserTest, ConvertType_StructEmpty) {
diff --git a/src/reader/spirv/parser_impl_get_decorations_test.cc b/src/reader/spirv/parser_impl_get_decorations_test.cc
index eb81a34..fc41141 100644
--- a/src/reader/spirv/parser_impl_get_decorations_test.cc
+++ b/src/reader/spirv/parser_impl_get_decorations_test.cc
@@ -60,6 +60,21 @@
p->SkipDumpingPending(kSkipReason);
}
+TEST_F(SpvParserGetDecorationsTest, GetDecorationsFor_Duplicate) {
+ auto p = parser(test::Assemble(R"(
+ OpDecorate %10 Block
+ OpDecorate %10 Block
+ %float = OpTypeFloat 32
+ %10 = OpTypeStruct %float
+ )"));
+ EXPECT_TRUE(p->BuildAndParseInternalModule());
+ auto decorations = p->GetDecorationsFor(10);
+ EXPECT_THAT(decorations,
+ UnorderedElementsAre(Decoration{SpvDecorationBlock}));
+ EXPECT_TRUE(p->error().empty());
+ p->SkipDumpingPending(kSkipReason);
+}
+
TEST_F(SpvParserGetDecorationsTest, GetDecorationsFor_MultiDecoration) {
auto p = parser(test::Assemble(R"(
OpDecorate %5 RelaxedPrecision
@@ -121,6 +136,21 @@
p->SkipDumpingPending(kSkipReason);
}
+TEST_F(SpvParserGetDecorationsTest, GetDecorationsForMember_Duplicate) {
+ auto p = parser(test::Assemble(R"(
+ OpMemberDecorate %10 0 RelaxedPrecision
+ OpMemberDecorate %10 0 RelaxedPrecision
+ %float = OpTypeFloat 32
+ %10 = OpTypeStruct %float
+ )"));
+ EXPECT_TRUE(p->BuildAndParseInternalModule()) << p->error();
+ auto decorations = p->GetDecorationsForMember(10, 0);
+ EXPECT_THAT(decorations,
+ UnorderedElementsAre(Decoration{SpvDecorationRelaxedPrecision}));
+ EXPECT_TRUE(p->error().empty());
+ p->SkipDumpingPending(kSkipReason);
+}
+
// TODO(dneto): Enable when ArrayStride is handled
TEST_F(SpvParserGetDecorationsTest,
DISABLED_GetDecorationsForMember_OneDecoration) {