From aa601387a23ea1b323922bb941d74867647fa6a2 Mon Sep 17 00:00:00 2001 From: David Neto Date: Thu, 16 Jul 2020 13:00:37 +0000 Subject: [PATCH] [spirv-reader] Support duplicate type definitions Affects structs, runtime arrays Bug: tint:3, tint:99 Change-Id: I9ca73f8f3f6395c829d134460ad4b1a9e50c3ec9 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/24720 Reviewed-by: dan sinclair --- src/reader/spirv/parser_impl.cc | 10 +-- src/reader/spirv/parser_impl.h | 20 +++++- .../spirv/parser_impl_named_types_test.cc | 65 ++++++++++++++++--- 3 files changed, 78 insertions(+), 17 deletions(-) diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index e4c13278fa..6794a378fc 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -281,7 +281,7 @@ ast::type::Type* ParserImpl::ConvertType(uint32_t type_id) { if (type != nullptr) { id_to_type_[type_id] = type; } - MaybeGenerateAlias(spirv_type); + MaybeGenerateAlias(type_id, spirv_type); return type; }; @@ -303,7 +303,7 @@ ast::type::Type* ParserImpl::ConvertType(uint32_t type_id) { case spvtools::opt::analysis::Type::kArray: return save(ConvertType(spirv_type->AsArray())); case spvtools::opt::analysis::Type::kStruct: - return save(ConvertType(spirv_type->AsStruct())); + return save(ConvertType(type_id, spirv_type->AsStruct())); case spvtools::opt::analysis::Type::kPointer: return save(ConvertType(spirv_type->AsPointer())); case spvtools::opt::analysis::Type::kFunction: @@ -671,8 +671,8 @@ bool ParserImpl::ApplyArrayDecorations( } ast::type::Type* ParserImpl::ConvertType( + uint32_t type_id, const spvtools::opt::analysis::Struct* struct_ty) { - const auto type_id = type_mgr_->GetId(struct_ty); // Compute the struct decoration. auto struct_decorations = this->GetDecorationsFor(type_id); auto ast_struct_decoration = ast::StructDecoration::kNone; @@ -757,11 +757,11 @@ bool ParserImpl::RegisterTypes() { return success_; } -void ParserImpl::MaybeGenerateAlias(const spvtools::opt::analysis::Type* type) { +void ParserImpl::MaybeGenerateAlias(uint32_t type_id, + const spvtools::opt::analysis::Type* type) { if (!success_) { return; } - const auto type_id = type_mgr_->GetId(type); // We only care about struct, arrays, and runtime arrays. switch (type->kind()) { diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index c4b717fbb7..281adaa118 100644 --- a/src/reader/spirv/parser_impl.h +++ b/src/reader/spirv/parser_impl.h @@ -146,8 +146,10 @@ class ParserImpl : Reader { /// - decorated arrays and runtime arrays /// TODO(dneto): I expect images and samplers to require names as well. /// This is a no-op if the parser has already failed. + /// @param type_id the SPIR-V ID for the type /// @param type the type that might get an alias - void MaybeGenerateAlias(const spvtools::opt::analysis::Type* type); + void MaybeGenerateAlias(uint32_t type_id, + const spvtools::opt::analysis::Type* type); /// @returns the fail stream object FailStream& fail_stream() { return fail_stream_; } @@ -324,14 +326,28 @@ class ParserImpl : Reader { /// Converts a specific SPIR-V type to a Tint type. Matrix case ast::type::Type* ConvertType(const spvtools::opt::analysis::Matrix* mat_ty); /// Converts a specific SPIR-V type to a Tint type. RuntimeArray case + /// @param rtarr_ty the Tint type ast::type::Type* ConvertType( const spvtools::opt::analysis::RuntimeArray* rtarr_ty); /// Converts a specific SPIR-V type to a Tint type. Array case + /// @param arr_ty the Tint type ast::type::Type* ConvertType(const spvtools::opt::analysis::Array* arr_ty); - /// Converts a specific SPIR-V type to a Tint type. Struct case + /// Converts a specific SPIR-V type to a Tint type. Struct case. + /// SPIR-V allows distinct struct type definitions for two OpTypeStruct + /// that otherwise have the same set of members (and struct and member + /// decorations). However, the SPIRV-Tools always produces a unique + /// |spvtools::opt::analysis::Struct| object in these cases. For this type + /// conversion, we need to have the original SPIR-V ID because we can't always + /// recover it from the optimizer's struct type object. This also lets us + /// preserve member names, which are given by OpMemberName which is normally + /// not significant to the optimizer's module representation. + /// @param type_id the SPIR-V ID for the type. + /// @param struct_ty the Tint type ast::type::Type* ConvertType( + uint32_t type_id, const spvtools::opt::analysis::Struct* struct_ty); /// Converts a specific SPIR-V type to a Tint type. Pointer case + /// @param ptr_ty the Tint type ast::type::Type* ConvertType(const spvtools::opt::analysis::Pointer* ptr_ty); /// Applies SPIR-V decorations to the given array or runtime-array type. diff --git a/src/reader/spirv/parser_impl_named_types_test.cc b/src/reader/spirv/parser_impl_named_types_test.cc index 0c6ba73cc5..1784c043dd 100644 --- a/src/reader/spirv/parser_impl_named_types_test.cc +++ b/src/reader/spirv/parser_impl_named_types_test.cc @@ -53,38 +53,83 @@ TEST_F(SpvParserTest, NamedTypes_NamedStruct) { EXPECT_THAT(p->module().to_str(), HasSubstr("mystruct -> __struct_")); } -// TODO(dneto): Enable this when array types can have ArrayStride -TEST_F(SpvParserTest, DISABLED_NamedTypes_AnonArrayWithDecoration) { +TEST_F(SpvParserTest, NamedTypes_Dup_EmitBoth) { auto* p = parser(test::Assemble(R"( - OpDecorate %arr ArrayStride 16 %uint = OpTypeInt 32 0 - %uint_3 = OpConstant %uint 3 - %arr = OpTypeArray %uint %uint_3 + %s = OpTypeStruct %uint %uint + %s2 = OpTypeStruct %uint %uint )")); - EXPECT_TRUE(p->BuildAndParseInternalModule()); - EXPECT_THAT(p->module().to_str(), HasSubstr("Arr -> __array__u32")); + EXPECT_TRUE(p->BuildAndParseInternalModule()) << p->error(); + EXPECT_THAT(p->module().to_str(), + HasSubstr("S -> __struct_S\nS_1 -> __struct_S_1")); } // TODO(dneto): Should we make an alias for an un-decoratrd array with // an OpName? -TEST_F(SpvParserTest, NamedTypes_AnonRTArray) { +TEST_F(SpvParserTest, NamedTypes_AnonRTArrayWithDecoration) { + // Runtime arrays are always in SSBO, and those are always laid out. auto* p = parser(test::Assemble(R"( + OpDecorate %arr ArrayStride 8 %uint = OpTypeInt 32 0 %arr = OpTypeRuntimeArray %uint )")); EXPECT_TRUE(p->BuildAndParseInternalModule()); - EXPECT_THAT(p->module().to_str(), HasSubstr("RTArr -> __array__u32")); + // TODO(dneto): this is a string collision with array + // https://bugs.chromium.org/p/tint/issues/detail?id=102 + EXPECT_THAT(p->module().to_str(), HasSubstr("RTArr -> __array__u32_8\n")); +} + +TEST_F(SpvParserTest, NamedTypes_AnonRTArray_Dup_EmitBoth) { + auto* p = parser(test::Assemble(R"( + OpDecorate %arr ArrayStride 8 + OpDecorate %arr2 ArrayStride 8 + %uint = OpTypeInt 32 0 + %arr = OpTypeRuntimeArray %uint + %arr2 = OpTypeRuntimeArray %uint + )")); + EXPECT_TRUE(p->BuildAndParseInternalModule()); + EXPECT_THAT( + p->module().to_str(), + HasSubstr("RTArr -> __array__u32_8\nRTArr_1 -> __array__u32_8\n")); } TEST_F(SpvParserTest, NamedTypes_NamedRTArray) { auto* p = parser(test::Assemble(R"( OpName %arr "myrtarr" + OpDecorate %arr ArrayStride 8 %uint = OpTypeInt 32 0 %arr = OpTypeRuntimeArray %uint )")); EXPECT_TRUE(p->BuildAndParseInternalModule()); - EXPECT_THAT(p->module().to_str(), HasSubstr("myrtarr -> __array__u32")); + EXPECT_THAT(p->module().to_str(), HasSubstr("myrtarr -> __array__u32_8\n")); +} + +TEST_F(SpvParserTest, NamedTypes_NamedArray) { + auto* p = parser(test::Assemble(R"( + OpName %arr "myarr" + OpDecorate %arr ArrayStride 8 + %uint = OpTypeInt 32 0 + %uint_5 = OpConstant %uint 5 + %arr = OpTypeArray %uint %uint_5 + %arr2 = OpTypeArray %uint %uint_5 + )")); + EXPECT_TRUE(p->BuildAndParseInternalModule()); + EXPECT_THAT(p->module().to_str(), HasSubstr("myarr -> __array__u32_5_8")); +} + +TEST_F(SpvParserTest, NamedTypes_AnonArray_Dup_EmitBoth) { + auto* p = parser(test::Assemble(R"( + OpDecorate %arr ArrayStride 8 + OpDecorate %arr2 ArrayStride 8 + %uint = OpTypeInt 32 0 + %uint_5 = OpConstant %uint 5 + %arr = OpTypeArray %uint %uint_5 + %arr2 = OpTypeArray %uint %uint_5 + )")); + EXPECT_TRUE(p->BuildAndParseInternalModule()); + EXPECT_THAT(p->module().to_str(), + HasSubstr("Arr -> __array__u32_5_8\nArr_1 -> __array__u32_5_8")); } // TODO(dneto): Handle arrays sized by a spec constant.