[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 <dsinclair@chromium.org>
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index e4c1327..6794a37 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/src/reader/spirv/parser_impl.cc
@@ -281,7 +281,7 @@
if (type != nullptr) {
id_to_type_[type_id] = type;
}
- MaybeGenerateAlias(spirv_type);
+ MaybeGenerateAlias(type_id, spirv_type);
return type;
};
@@ -303,7 +303,7 @@
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 @@
}
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 @@
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 c4b717f..281adaa 100644
--- a/src/reader/spirv/parser_impl.h
+++ b/src/reader/spirv/parser_impl.h
@@ -146,8 +146,10 @@
/// - 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 @@
/// 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 0c6ba73..1784c04 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 @@
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<u32,8>
+ // 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.
