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>

src/reader/spirv/parser_impl.cc

@@ -388,16 +388,20 @@ const Type* ParserImpl::ConvertType(uint32_t type_id, PtrAs ptr_as) {
 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;
+    auto decoration_kind = inst->GetSingleWordInOperand(1);
-    inst->ToBinaryWithoutAttachedDebugInsts(&inst_as_words);
+    if (visited.emplace(decoration_kind).second) {
-    Decoration d(inst_as_words.begin() + 2, inst_as_words.end());
+      std::vector<uint32_t> inst_as_words;
-    result.push_back(d);
+      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 @@ DecorationList ParserImpl::GetDecorationsForMember(
     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
+    auto decoration_kind = inst->GetSingleWordInOperand(2);
-    std::vector<uint32_t> inst_as_words;
+    if (visited.emplace(decoration_kind).second) {
-    inst->ToBinaryWithoutAttachedDebugInsts(&inst_as_words);
+      std::vector<uint32_t> inst_as_words;
-    Decoration d(inst_as_words.begin() + 3, inst_as_words.end());
+      inst->ToBinaryWithoutAttachedDebugInsts(&inst_as_words);
-    result.push_back(d);
+      Decoration d(inst_as_words.begin() + 3, inst_as_words.end());
+      result.push_back(d);
+    }
   }
   return result;
 }
@@ -1046,7 +1054,6 @@ const Type* ParserImpl::ConvertType(
 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 @@ bool ParserImpl::ParseArrayDecorations(
         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

src/reader/spirv/parser_impl.h

@@ -218,16 +218,16 @@ class ParserImpl : Reader {
   /// 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
+  /// Gets the list of unique decorations for a SPIR-V result ID.  Returns an
-  /// vector if the ID is not a result ID, or if no decorations target that ID.
+  /// empty vector if the ID is not a result ID, or if no decorations target
-  /// The internal representation must have already been built.
+  /// 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
+  /// Gets the list of unique decorations for the member of a struct.  Returns
-  /// list if the `id` is not the ID of a struct, or if the member index is out
+  /// an empty list if the `id` is not the ID of a struct, or if the member
-  /// of range, or if the target member has no decorations.
+  /// index is out of range, or if the target member has no decorations. The
-  /// The internal representation must have already been built.
+  /// 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

src/reader/spirv/parser_impl_convert_type_test.cc

@@ -401,8 +401,8 @@ TEST_F(SpvParserTest, ConvertType_RuntimeArray_ArrayStride_Valid) {
   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_ZeroIsError) {
 }
 
 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);
+  ASSERT_NE(type, nullptr);
-  EXPECT_THAT(p->error(),
+  auto* arr_type = type->UnwrapAll()->As<Array>();
-              Eq("invalid array type ID 10: multiple ArrayStride decorations"));
+  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 @@ TEST_F(SpvParserTest, ConvertType_ArrayStride_ZeroIsError) {
               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 @@ TEST_F(SpvParserTest, ConvertType_ArrayStride_SpecifiedTwiceIsError) {
   EXPECT_TRUE(p->BuildInternalModule());
 
   auto* type = p->ConvertType(10);
-  ASSERT_EQ(type, nullptr);
+  ASSERT_NE(type, nullptr);
-  EXPECT_THAT(p->error(),
+  EXPECT_TRUE(type->UnwrapAll()->Is<Array>());
-              Eq("invalid array type ID 10: multiple ArrayStride decorations"));
+  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) {

src/reader/spirv/parser_impl_get_decorations_test.cc

@@ -60,6 +60,21 @@ TEST_F(SpvParserGetDecorationsTest, GetDecorationsFor_OneDecoration) {
   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 @@ TEST_F(SpvParserGetDecorationsTest, GetDecorationsForMember_RelaxedPrecision) {
   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) {