diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 544fcfe17e..4f28c1972f 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/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 visited; for (const auto* inst : decorations) { if (inst->opcode() != SpvOpDecorate) { continue; } // Example: OpDecorate %struct_id Block // Example: OpDecorate %array_ty ArrayStride 16 - std::vector 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 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 @@ DecorationList ParserImpl::GetDecorationsForMember( uint32_t member_index) const { DecorationList result; const auto& decorations = deco_mgr_->GetDecorationsFor(id, true); + std::unordered_set 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 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 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 @@ 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 diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index 9895439dd8..de9f71a205 100644 --- a/src/reader/spirv/parser_impl.h +++ b/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 - /// 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 1828362001..8ad4177467 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 @@ TEST_F(SpvParserTest, ConvertType_RuntimeArray_ArrayStride_Valid) { auto* type = p->ConvertType(10); ASSERT_NE(type, nullptr); auto* arr_type = type->UnwrapAll()->As(); - 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); - EXPECT_THAT(p->error(), - Eq("invalid array type ID 10: multiple ArrayStride decorations")); + ASSERT_NE(type, nullptr); + auto* arr_type = type->UnwrapAll()->As(); + 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); - EXPECT_THAT(p->error(), - Eq("invalid array type ID 10: multiple ArrayStride decorations")); + ASSERT_NE(type, nullptr); + EXPECT_TRUE(type->UnwrapAll()->Is()); + auto* arr_type = type->UnwrapAll()->As(); + 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 eb81a349d1..fc41141c2f 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 @@ 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) {