From 7aac7b521e2f26294dbda547071135ad9155fa12 Mon Sep 17 00:00:00 2001 From: David Neto Date: Mon, 21 Jun 2021 21:45:36 +0000 Subject: [PATCH] spirv-reader: Handle vector insert dynamic into a hoisted variable VectorInsertDynamic normaly inserts a temporary variable. But if the source vector is already a hoisted variable, then reuse that instead. This avoids defining the same name twice. Bug: tint:804 Change-Id: I69c20c11d462c148261bb0646db698dc7850495d Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/55362 Auto-Submit: David Neto Kokoro: Kokoro Commit-Queue: David Neto Reviewed-by: Ben Clayton --- src/reader/spirv/function.cc | 74 ++++++++++++++-------- src/reader/spirv/function_var_test.cc | 90 +++++++++++++++++++++++++-- 2 files changed, 133 insertions(+), 31 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index ec9781e59a..0b0054f66c 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -5434,45 +5434,68 @@ bool FunctionEmitter::MakeVectorInsertDynamic( const spvtools::opt::Instruction& inst) { // For // %result = OpVectorInsertDynamic %type %src_vector %component %index - // generate statements like this: + // there are two cases. + // + // Case 1: + // The %src_vector value has already been hoisted into a variable. + // In this case, assign %src_vector to that variable, then write the + // component into the right spot: + // + // hoisted = src_vector; + // hoisted[index] = component; + // + // Case 2: + // The %src_vector value is not hoisted. In this case, make a temporary + // variable with the %src_vector contents, then write the component, + // and then make a let-declaration that reads the value out: // // var temp : type = src_vector; // temp[index] = component; // let result : type = temp; // - // Then use result everywhere the original SPIR-V id is used. Using a const - // like this avoids constantly reloading the value many times. + // Then use result everywhere the original SPIR-V id is used. Using a const + // like this avoids constantly reloading the value many times. - // TODO(dneto): crbug.com/tint/804: handle the case where %src_vector is - // has been hoisted into a variable. - - auto* ast_type = parser_impl_.ConvertType(inst.type_id()); + auto* type = parser_impl_.ConvertType(inst.type_id()); auto src_vector = MakeOperand(inst, 0); auto component = MakeOperand(inst, 1); auto index = MakeOperand(inst, 2); - // Synthesize the temporary variable. - // It doesn't correspond to a SPIR-V ID, so we don't use the ordinary - // API in parser_impl_. - auto result_name = namer_.Name(inst.result_id()); - auto temp_name = namer_.MakeDerivedName(result_name); - auto registered_temp_name = builder_.Symbols().Register(temp_name); + std::string var_name; + auto original_value_name = namer_.Name(inst.result_id()); + const bool hoisted = WriteIfHoistedVar(inst, src_vector); + if (hoisted) { + // The variable was already declared in an earlier block. + var_name = original_value_name; + // Assign the source vector value to it. + builder_.Assign({}, builder_.Expr(var_name), src_vector.expr); + } else { + // Synthesize the temporary variable. + // It doesn't correspond to a SPIR-V ID, so we don't use the ordinary + // API in parser_impl_. + var_name = namer_.MakeDerivedName(original_value_name); - auto* temp_var = create( - Source{}, registered_temp_name, ast::StorageClass::kNone, - ast::Access::kUndefined, ast_type->Build(builder_), false, - src_vector.expr, ast::DecorationList{}); - AddStatement(create(Source{}, temp_var)); + auto* temp_var = builder_.Var(var_name, type->Build(builder_), + ast::StorageClass::kNone, src_vector.expr); + + AddStatement(builder_.Decl({}, temp_var)); + } auto* lhs = create( - Source{}, create(registered_temp_name), - index.expr); + Source{}, builder_.Expr(var_name), index.expr); + if (!lhs) { + return false; + } - AddStatement(create(Source{}, lhs, component.expr)); + AddStatement(builder_.Assign(lhs, component.expr)); - return EmitConstDefinition( - inst, - {ast_type, create(registered_temp_name)}); + if (hoisted) { + // The hoisted variable itself stands for this result ID. + return success(); + } + // Create a new let-declaration that is initialized by the contents + // of the temporary variable. + return EmitConstDefinition(inst, {type, builder_.Expr(var_name)}); } bool FunctionEmitter::MakeCompositeInsert( @@ -5538,8 +5561,7 @@ bool FunctionEmitter::MakeCompositeInsert( return false; } - AddStatement( - create(Source{}, lhs.expr, component.expr)); + AddStatement(builder_.Assign(lhs.expr, component.expr)); if (hoisted) { // The hoisted variable itself stands for this result ID. diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc index 36ba2d09b2..bd169518b8 100644 --- a/src/reader/spirv/function_var_test.cc +++ b/src/reader/spirv/function_var_test.cc @@ -60,6 +60,8 @@ std::string CommonTypes() { %uint_1 = OpConstant %uint 1 %int_m1 = OpConstant %int -1 %int_0 = OpConstant %int 0 + %int_1 = OpConstant %int 1 + %int_3 = OpConstant %int 3 %uint_2 = OpConstant %uint 2 %uint_3 = OpConstant %uint 3 %uint_4 = OpConstant %uint 4 @@ -2286,7 +2288,7 @@ TEST_F(SpvParserFunctionVarTest, auto got = ToString(p->builder(), fe.ast_body()); auto* expect = R"(VariableDeclStatement{ Variable{ - x_38_phi + x_41_phi none undefined __u32 @@ -2312,14 +2314,14 @@ Switch{ Else{ { Assignment{ - Identifier[not set]{x_38_phi} + Identifier[not set]{x_41_phi} ScalarConstructor[not set]{0u} } Break{} } } Assignment{ - Identifier[not set]{x_38_phi} + Identifier[not set]{x_41_phi} ScalarConstructor[not set]{1u} } } @@ -2327,12 +2329,12 @@ Switch{ } VariableDeclStatement{ VariableConst{ - x_38 + x_41 none undefined __u32 { - Identifier[not set]{x_38_phi} + Identifier[not set]{x_41_phi} } } } @@ -2602,6 +2604,84 @@ Return{} EXPECT_EQ(got, expected); } +TEST_F(SpvParserFunctionVarTest, EmitStatement_Hoist_VectorInsertDynamic) { + // Spawned from crbug.com/tint/804 + const auto assembly = Preamble() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + OpSelectionMerge %50 None + OpBranchConditional %true %20 %30 + + %20 = OpLabel + %200 = OpVectorInsertDynamic %v2int %v2int_null %int_3 %int_1 + OpBranch %50 + + %30 = OpLabel + OpReturn + + %50 = OpLabel ; dominated by %20, but %200 needs to be hoisted + %201 = OpCopyObject %v2int %200 + OpReturn + OpFunctionEnd +)"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly; + auto fe = p->function_emitter(100); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + + const auto got = ToString(p->builder(), fe.ast_body()); + const auto* expected = R"(VariableDeclStatement{ + Variable{ + x_200 + none + undefined + __vec_2__i32 + } +} +If{ + ( + ScalarConstructor[not set]{true} + ) + { + Assignment{ + Identifier[not set]{x_200} + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{0} + ScalarConstructor[not set]{0} + } + } + Assignment{ + ArrayAccessor[not set]{ + Identifier[not set]{x_200} + ScalarConstructor[not set]{1} + } + ScalarConstructor[not set]{3} + } + } +} +Else{ + { + Return{} + } +} +VariableDeclStatement{ + VariableConst{ + x_201 + none + undefined + __vec_2__i32 + { + Identifier[not set]{x_200} + } + } +} +Return{} +)"; + EXPECT_EQ(got, expected) << got; +} + } // namespace } // namespace spirv } // namespace reader