spirv-reader: Avoid struct type dedup confusion

When generating the store type for a variable, follow SPIR-V
instructions directly.  Avoid using the SPIRV-Tools optimizer because
it deduplicates structures that differ only in non-semantic annotation (e.g. member names)

Bug: tint:737
Change-Id: I9503be4ae0a95a517d595da23e6ce2397e4fc9c1
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/53560
Auto-Submit: David Neto <dneto@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: James Price <jrprice@google.com>

src/reader/spirv/function.cc

@@ -1116,20 +1116,16 @@ bool FunctionEmitter::ParseFunctionDeclaration(FunctionDeclaration* decl) {
 const Type* FunctionEmitter::GetVariableStoreType(
     const spvtools::opt::Instruction& var_decl_inst) {
   const auto type_id = var_decl_inst.type_id();
-  auto* var_ref_type = type_mgr_->GetType(type_id);
+  // Normally we use the SPIRV-Tools optimizer to manage types.
-  if (!var_ref_type) {
+  // But when two struct types have the same member types and decorations,
-    Fail() << "internal error: variable type id " << type_id
+  // but differ only in member names, the two struct types will be
-           << " has no registered type";
+  // represented by a single common internal struct type.
-    return nullptr;
+  // So avoid the optimizer's representation and instead follow the
-  }
+  // SPIR-V instructions themselves.
-  auto* var_ref_ptr_type = var_ref_type->AsPointer();
+  const auto* ptr_ty = def_use_mgr_->GetDef(type_id);
-  if (!var_ref_ptr_type) {
+  const auto store_ty_id = ptr_ty->GetSingleWordInOperand(1);
-    Fail() << "internal error: variable type id " << type_id
+  const auto* result = parser_impl_.ConvertType(store_ty_id);
-           << " is not a pointer type";
+  return result;
-    return nullptr;
-  }
-  auto var_store_type_id = type_mgr_->GetId(var_ref_ptr_type->pointee_type());
-  return parser_impl_.ConvertType(var_store_type_id);
 }
 
 bool FunctionEmitter::EmitBody() {

src/reader/spirv/function_composite_test.cc

@@ -466,12 +466,19 @@ TEST_F(SpvParserTest_CompositeExtract, Struct) {
 }
 
 TEST_F(SpvParserTest_CompositeExtract, Struct_DifferOnlyInMemberName) {
-  const auto assembly = Caps() +
+  const std::string assembly = R"(
-                        R"(
+     OpCapability Shader
-      OpMemberName %s0 0 "algo"
+     OpMemoryModel Logical Simple
-      OpMemberName %s1 0 "rithm"
+     OpEntryPoint Vertex %100 "main"
-)" + CommonTypes() +
+
-                        R"(
+     OpMemberName %s0 0 "algo"
+     OpMemberName %s1 0 "rithm"
+
+     %void = OpTypeVoid
+     %voidfn = OpTypeFunction %void
+
+     %uint = OpTypeInt 32 0
+
      %s0 = OpTypeStruct %uint
      %s1 = OpTypeStruct %uint
      %ptr0 = OpTypePointer Function %s0
@@ -913,12 +920,23 @@ VariableDeclStatement{
 }
 
 TEST_F(SpvParserTest_CompositeInsert, Struct_DifferOnlyInMemberName) {
-  const auto assembly = Caps() +
+  const std::string assembly = R"(
-                        R"(
+     OpCapability Shader
-      OpMemberName %s0 0 "algo"
+     OpMemoryModel Logical Simple
-      OpMemberName %s1 0 "rithm"
+     OpEntryPoint Vertex %100 "main"
-)" + CommonTypes() +
+
-                        R"(
+     OpName %var0 "var0"
+     OpName %var1 "var1"
+     OpMemberName %s0 0 "algo"
+     OpMemberName %s1 0 "rithm"
+
+     %void = OpTypeVoid
+     %voidfn = OpTypeFunction %void
+
+     %uint = OpTypeInt 32 0
+     %uint_10 = OpConstant %uint 10
+     %uint_11 = OpConstant %uint 11
+
      %s0 = OpTypeStruct %uint
      %s1 = OpTypeStruct %uint
      %ptr0 = OpTypePointer Function %s0
@@ -931,7 +949,7 @@ TEST_F(SpvParserTest_CompositeInsert, Struct_DifferOnlyInMemberName) {
      %1 = OpLoad %s0 %var0
      %2 = OpCompositeInsert %s0 %uint_10 %1 0
      %3 = OpLoad %s1 %var1
-     %4 = OpCompositeInsert %s1 %uint_10 %3 0
+     %4 = OpCompositeInsert %s1 %uint_11 %3 0
      OpReturn
      OpFunctionEnd
   )";
@@ -939,21 +957,21 @@ TEST_F(SpvParserTest_CompositeInsert, Struct_DifferOnlyInMemberName) {
   ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
   auto fe = p->function_emitter(100);
   EXPECT_TRUE(fe.EmitBody()) << p->error();
-  auto body_str = ToString(p->builder(), fe.ast_body());
+  const auto got = ToString(p->builder(), fe.ast_body());
-  EXPECT_THAT(body_str, HasSubstr(R"(VariableDeclStatement{
+  const std::string expected = R"(VariableDeclStatement{
   Variable{
-    x_40
+    var0
     none
     undefined
-    __type_name_S_2
+    __type_name_S
   }
 }
 VariableDeclStatement{
   Variable{
-    x_41
+    var1
     none
     undefined
-    __type_name_S_2
+    __type_name_S_1
   }
 }
 VariableDeclStatement{
@@ -961,9 +979,9 @@ VariableDeclStatement{
     x_1
     none
     undefined
-    __type_name_S_2
+    __type_name_S
     {
-      Identifier[not set]{x_40}
+      Identifier[not set]{var0}
     }
   }
 }
@@ -972,7 +990,7 @@ VariableDeclStatement{
     x_2_1
     none
     undefined
-    __type_name_S_1
+    __type_name_S
     {
       Identifier[not set]{x_1}
     }
@@ -990,7 +1008,7 @@ VariableDeclStatement{
     x_2
     none
     undefined
-    __type_name_S_1
+    __type_name_S
     {
       Identifier[not set]{x_2_1}
     }
@@ -1001,9 +1019,9 @@ VariableDeclStatement{
     x_3
     none
     undefined
-    __type_name_S_2
+    __type_name_S_1
     {
-      Identifier[not set]{x_41}
+      Identifier[not set]{var1}
     }
   }
 }
@@ -1012,7 +1030,7 @@ VariableDeclStatement{
     x_4_1
     none
     undefined
-    __type_name_S_2
+    __type_name_S_1
     {
       Identifier[not set]{x_3}
     }
@@ -1023,50 +1041,22 @@ Assignment{
     Identifier[not set]{x_4_1}
     Identifier[not set]{rithm}
   }
-  ScalarConstructor[not set]{10u}
+  ScalarConstructor[not set]{11u}
 }
 VariableDeclStatement{
   VariableConst{
     x_4
     none
     undefined
-    __type_name_S_2
+    __type_name_S_1
     {
       Identifier[not set]{x_4_1}
     }
   }
 }
-)")) << body_str;
+Return{}
-  EXPECT_THAT(body_str, HasSubstr(R"(VariableDeclStatement{
+)";
-  Variable{
+  EXPECT_EQ(got, expected) << got;
-    x_4_1
-    none
-    undefined
-    __type_name_S_2
-    {
-      Identifier[not set]{x_3}
-    }
-  }
-}
-Assignment{
-  MemberAccessor[not set]{
-    Identifier[not set]{x_4_1}
-    Identifier[not set]{rithm}
-  }
-  ScalarConstructor[not set]{10u}
-}
-VariableDeclStatement{
-  VariableConst{
-    x_4
-    none
-    undefined
-    __type_name_S_2
-    {
-      Identifier[not set]{x_4_1}
-    }
-  }
-})")) << body_str;
-  p->SkipDumpingPending("crbug.com/tint/863");
 }
 
 TEST_F(SpvParserTest_CompositeInsert, Struct_IndexTooBigError) {

src/reader/spirv/function_var_test.cc

@@ -689,7 +689,7 @@ TEST_F(SpvParserFunctionVarTest, EmitFunctionVariables_StructInitializer_Null) {
 TEST_F(SpvParserFunctionVarTest,
        EmitFunctionVariables_Decorate_RelaxedPrecision) {
   // RelaxedPrecisionis dropped
-  auto p = parser(test::Assemble(Caps({"myvar"}) + R"(
+  const auto assembly = Caps({"myvar"}) + R"(
      OpDecorate %myvar RelaxedPrecision
 
      %float = OpTypeFloat 32
@@ -703,7 +703,8 @@ TEST_F(SpvParserFunctionVarTest,
      %myvar = OpVariable %ptr Function
      OpReturn
      OpFunctionEnd
-  )"));
+  )";
+  auto p = parser(test::Assemble(assembly));
   ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions());
   auto fe = p->function_emitter(100);
   EXPECT_TRUE(fe.EmitFunctionVariables());
@@ -757,6 +758,55 @@ TEST_F(SpvParserFunctionVarTest,
 )") << got;
 }
 
+TEST_F(SpvParserFunctionVarTest,
+       EmitFunctionVariables_StructDifferOnlyInMemberName) {
+  auto p = parser(test::Assemble(R"(
+      OpCapability Shader
+      OpMemoryModel Logical Simple
+      OpEntryPoint Vertex %100 "main"
+      OpName %_struct_5 "S"
+      OpName %_struct_6 "S"
+      OpMemberName %_struct_5 0 "algo"
+      OpMemberName %_struct_6 0 "rithm"
+
+      %void = OpTypeVoid
+      %voidfn = OpTypeFunction %void
+      %uint = OpTypeInt 32 0
+
+      %_struct_5 = OpTypeStruct %uint
+      %_struct_6 = OpTypeStruct %uint
+      %_ptr_Function__struct_5 = OpTypePointer Function %_struct_5
+      %_ptr_Function__struct_6 = OpTypePointer Function %_struct_6
+      %100 = OpFunction %void None %voidfn
+      %39 = OpLabel
+      %40 = OpVariable %_ptr_Function__struct_5 Function
+      %41 = OpVariable %_ptr_Function__struct_6 Function
+      OpReturn
+      OpFunctionEnd)"));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions());
+  auto fe = p->function_emitter(100);
+  EXPECT_TRUE(fe.EmitFunctionVariables());
+
+  const auto got = ToString(p->builder(), fe.ast_body());
+  EXPECT_THAT(got, HasSubstr(R"(VariableDeclStatement{
+  Variable{
+    x_40
+    none
+    undefined
+    __type_name_S
+  }
+}
+VariableDeclStatement{
+  Variable{
+    x_41
+    none
+    undefined
+    __type_name_S_1
+  }
+}
+)"));
+}
+
 TEST_F(SpvParserFunctionVarTest,
        EmitStatement_CombinatorialValue_Defer_UsedOnceSameConstruct) {
   auto assembly = Preamble() + R"(