From 0d6d905bc08ea4149c71f69174ab5887067fc632 Mon Sep 17 00:00:00 2001 From: David Neto Date: Wed, 9 Jun 2021 14:47:09 +0000 Subject: [PATCH] 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 Kokoro: Kokoro Commit-Queue: David Neto Reviewed-by: James Price --- src/reader/spirv/function.cc | 24 ++--- src/reader/spirv/function_composite_test.cc | 108 +++++++++----------- src/reader/spirv/function_var_test.cc | 54 +++++++++- 3 files changed, 111 insertions(+), 75 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index e5a97f06f7..9d6a615a11 100644 --- a/src/reader/spirv/function.cc +++ b/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); - if (!var_ref_type) { - Fail() << "internal error: variable type id " << type_id - << " has no registered type"; - return nullptr; - } - auto* var_ref_ptr_type = var_ref_type->AsPointer(); - if (!var_ref_ptr_type) { - Fail() << "internal error: variable type id " << type_id - << " is not a pointer type"; - return nullptr; - } - auto var_store_type_id = type_mgr_->GetId(var_ref_ptr_type->pointee_type()); - return parser_impl_.ConvertType(var_store_type_id); + // Normally we use the SPIRV-Tools optimizer to manage types. + // But when two struct types have the same member types and decorations, + // but differ only in member names, the two struct types will be + // represented by a single common internal struct type. + // So avoid the optimizer's representation and instead follow the + // SPIR-V instructions themselves. + const auto* ptr_ty = def_use_mgr_->GetDef(type_id); + const auto store_ty_id = ptr_ty->GetSingleWordInOperand(1); + const auto* result = parser_impl_.ConvertType(store_ty_id); + return result; } bool FunctionEmitter::EmitBody() { diff --git a/src/reader/spirv/function_composite_test.cc b/src/reader/spirv/function_composite_test.cc index cffac8a3d4..aca7eba228 100644 --- a/src/reader/spirv/function_composite_test.cc +++ b/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() + - R"( - OpMemberName %s0 0 "algo" - OpMemberName %s1 0 "rithm" -)" + CommonTypes() + - R"( + const std::string assembly = R"( + OpCapability Shader + OpMemoryModel Logical Simple + OpEntryPoint Vertex %100 "main" + + 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() + - R"( - OpMemberName %s0 0 "algo" - OpMemberName %s1 0 "rithm" -)" + CommonTypes() + - R"( + const std::string assembly = R"( + OpCapability Shader + OpMemoryModel Logical Simple + OpEntryPoint Vertex %100 "main" + + 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()); - EXPECT_THAT(body_str, HasSubstr(R"(VariableDeclStatement{ + const auto got = ToString(p->builder(), fe.ast_body()); + 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; - EXPECT_THAT(body_str, HasSubstr(R"(VariableDeclStatement{ - Variable{ - 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"); +Return{} +)"; + EXPECT_EQ(got, expected) << got; } TEST_F(SpvParserTest_CompositeInsert, Struct_IndexTooBigError) { diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc index 071d30c42d..ca6f5b3124 100644 --- a/src/reader/spirv/function_var_test.cc +++ b/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"(