[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>

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()) {

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.

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<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.