From c5d65cc91795e078160af11bbd848b8c1c31e59b Mon Sep 17 00:00:00 2001 From: David Neto Date: Sat, 29 Aug 2020 16:11:02 +0000 Subject: [PATCH] [spirv-reader] Don't hoist pointers that are already in scope. Bug: tint:3, tint:213 Change-Id: I79410c670b8690c1f1ea86b7b9427f272a4ebbbb Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27720 Reviewed-by: dan sinclair Commit-Queue: dan sinclair --- src/reader/spirv/function.cc | 30 ++++++++++-- src/reader/spirv/function.h | 6 ++- src/reader/spirv/function_var_test.cc | 70 ++++++++++++++++++++++++++- 3 files changed, 100 insertions(+), 6 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 5fb6b2c62a..724ff47598 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -3247,12 +3247,21 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // Update the usage span for IDs used by this instruction. // But skip uses in OpPhi because they are handled differently. if (inst.opcode() != SpvOpPhi) { - inst.ForEachInId([this, block_pos](const uint32_t* id_ptr) { + inst.ForEachInId([this, block_pos, block_info](const uint32_t* id_ptr) { auto* def_info = GetDefInfo(*id_ptr); if (def_info) { def_info->num_uses++; def_info->last_use_pos = std::max(def_info->last_use_pos, block_pos); + + // Determine whether this ID is defined in a different construct + // from this use. + const auto defining_block = block_order_[def_info->block_pos]; + const auto* def_in_construct = + GetBlockInfo(defining_block)->construct; + if (def_in_construct != block_info->construct) { + def_info->used_in_another_construct = true; + } } }); } @@ -3317,8 +3326,6 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { const auto first_pos = def_info->block_pos; const auto last_use_pos = def_info->last_use_pos; - const auto* const construct_with_last_use = - GetBlockInfo(block_order_[last_use_pos])->construct; const auto* def_in_construct = GetBlockInfo(block_order_[first_pos])->construct; // A definition in the first block of an kIfSelection or kSwitchSelection @@ -3331,7 +3338,22 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { } } - if (def_in_construct != construct_with_last_use) { + bool should_hoist = false; + if (!def_in_construct->ContainsPos(last_use_pos)) { + // To satisfy scoping, we have to hoist the definition out to an enclosing + // construct. + should_hoist = true; + } else { + // Avoid moving combinatorial values across constructs. This is a + // simple heuristic to avoid changing the cost of an operation + // by moving it into or out of a loop, for example. + if ((def_info->storage_class == ast::StorageClass::kNone) && + def_info->used_in_another_construct) { + should_hoist = true; + } + } + + if (should_hoist) { const auto* enclosing_construct = GetEnclosingScope(first_pos, last_use_pos); if (enclosing_construct == def_in_construct) { diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index 6d3ebb3d6a..007c756e1e 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -223,6 +223,10 @@ struct DefInfo { /// at all. The "last" ordering is determined by the function block order. uint32_t last_use_pos = 0; + /// Is this value used in a construct other than the one in which it was + /// defined? + bool used_in_another_construct = false; + /// True if this ID requires a WGSL 'const' definition, due to context. It /// might get one anyway (so this is *not* an if-and-only-if condition). bool requires_named_const_def = false; @@ -248,7 +252,7 @@ struct DefInfo { std::string phi_var; /// The storage class to use for this value, if it is of pointer type. - /// This is required to carry a stroage class override from a storage + /// This is required to carry a storage class override from a storage /// buffer expressed in the old style (with Uniform storage class) /// that needs to be remapped to StorageBuffer storage class. /// This is kNone for non-pointers. diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc index 2867a74ec0..ebbac3dda8 100644 --- a/src/reader/spirv/function_var_test.cc +++ b/src/reader/spirv/function_var_test.cc @@ -728,7 +728,7 @@ TEST_F(SpvParserTest, EmitStatement_CombinatorialValue_Immediate_UsedOnceDifferentConstruct) { // Translation should not sink expensive operations into or out of control // flow. As a simple heuristic, don't move *any* combinatorial operation - // across any constrol flow. + // across any control flow. auto assembly = Preamble() + R"( %100 = OpFunction %void None %voidfn @@ -1182,6 +1182,74 @@ Return{} )")) << ToString(fe.ast_body()); } +TEST_F(SpvParserTest, + EmitStatement_CombinatorialNonPointer_Hoisting_DefAndUseFirstBlockIf) { + // In this test, both the defintion and the use are in the first block + // of an IfSelection. No hoisting occurs because hoisting is triggered + // on whether the defining construct contains the last use, rather than + // whether the two constructs are the same. + // + // This example has two SSA IDs which are tempting to hoist but should not: + // %1 is defined and used in the first block of an IfSelection. + // Do not hoist it. + auto assembly = Preamble() + R"( + %cond = OpConstantTrue %bool + + %100 = OpFunction %void None %voidfn + + ; in IfSelection construct, nested in Function construct + %10 = OpLabel + %1 = OpCopyObject %uint %uint_1 + %2 = OpCopyObject %uint %1 + OpSelectionMerge %99 None + OpBranchConditional %cond %20 %99 + + %20 = OpLabel ; in IfSelection construct + OpBranch %99 + + %99 = OpLabel + OpReturn + + OpFunctionEnd + )"; + auto* p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly; + FunctionEmitter fe(p, *spirv_function(100)); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + + // We don't hoist x_1 into its own mutable variable. It is emitted as + // a const definition. + EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(VariableDeclStatement{ + Variable{ + x_1 + none + __u32 + { + ScalarConstructor{1} + } + } +} +VariableDeclStatement{ + Variable{ + x_2 + none + __u32 + { + Identifier{x_1} + } + } +} +If{ + ( + ScalarConstructor{true} + ) + { + } +} +Return{} +)")) << ToString(fe.ast_body()); +} + TEST_F(SpvParserTest, EmitStatement_Phi_SingleBlockLoopIndex) { auto assembly = Preamble() + R"( %pty = OpTypePointer Private %uint