From 25b856aa64309e41c388aff5e9a9cf621a7b8f17 Mon Sep 17 00:00:00 2001 From: David Neto Date: Fri, 28 Aug 2020 19:58:12 +0000 Subject: [PATCH] [spirv-reader] Avoid certain hoisting cases If a definition occurs in the first block of an IfSelection or SwitchSelection, then it should count as belonging to the scope of the parent of that if or switch selection. This prevents some bad hoisting decisions. Bug: tint:3, tint:213 Change-Id: I89df5e8cbc163577e19e78c2bf393eb1eec4a0aa Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27660 Reviewed-by: dan sinclair Commit-Queue: David Neto --- src/reader/spirv/function.cc | 13 +- src/reader/spirv/function_var_test.cc | 253 ++++++++++++++++++++++++++ 2 files changed, 264 insertions(+), 2 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 3687bf9ba4..5fb6b2c62a 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -3317,10 +3317,19 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { const auto first_pos = def_info->block_pos; const auto last_use_pos = def_info->last_use_pos; - const auto* const def_in_construct = - GetBlockInfo(block_order_[first_pos])->construct; 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 + // occurs before the branch, and so that definition should count as + // having been defined at the scope of the parent construct. + if (first_pos == def_in_construct->begin_pos) { + if ((def_in_construct->kind == Construct::kIfSelection) || + (def_in_construct->kind == Construct::kSwitchSelection)) { + def_in_construct = def_in_construct->parent; + } + } if (def_in_construct != construct_with_last_use) { const auto* enclosing_construct = diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc index d5de2e11a5..2867a74ec0 100644 --- a/src/reader/spirv/function_var_test.cc +++ b/src/reader/spirv/function_var_test.cc @@ -929,6 +929,259 @@ Return{} )")) << ToString(fe.ast_body()); } +TEST_F( + SpvParserTest, + EmitStatement_CombinatorialNonPointer_Hoisting_DefFirstBlockIf_InFunction) { + // This is a hoisting case, where the definition is in the first block + // of an if selection construct. In this case the definition should count + // as being in the parent (enclosing) construct. + // + // The definition of %1 is in an IfSelection construct and also the enclosing + // Function construct, both of which start at block %10. For the purpose of + // determining the construct containing %10, go to the parent construct of + // the IfSelection. + auto assembly = Preamble() + R"( + %pty = OpTypePointer Private %uint + %200 = OpVariable %pty Private + %cond = OpConstantTrue %bool + + %100 = OpFunction %void None %voidfn + + ; in IfSelection construct, nested in Function construct + %10 = OpLabel + %1 = OpCopyObject %uint %uint_1 + OpSelectionMerge %99 None + OpBranchConditional %cond %20 %99 + + %20 = OpLabel ; in IfSelection construct + OpBranch %99 + + %99 = OpLabel + %3 = OpCopyObject %uint %1; in Function construct + OpStore %200 %3 + 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} + } + } +} +If{ + ( + ScalarConstructor{true} + ) + { + } +} +VariableDeclStatement{ + Variable{ + x_3 + none + __u32 + { + Identifier{x_1} + } + } +} +Assignment{ + Identifier{x_200} + Identifier{x_3} +} +Return{} +)")) << ToString(fe.ast_body()); +} + +TEST_F(SpvParserTest, + EmitStatement_CombinatorialNonPointer_Hoisting_DefFirstBlockIf_InIf) { + // This is like the previous case, but the IfSelection is nested inside + // another IfSelection. + // This tests that the hoisting algorithm goes to only one parent of + // the definining if-selection block, and doesn't jump all the way out + // to the Function construct that encloses everything. + // + // We should not hoist %1 because its definition should count as being + // in the outer IfSelection, not the inner IfSelection. + auto assembly = Preamble() + R"( + + %pty = OpTypePointer Private %uint + %200 = OpVariable %pty Private + %cond = OpConstantTrue %bool + + %100 = OpFunction %void None %voidfn + + ; outer IfSelection + %10 = OpLabel + OpSelectionMerge %99 None + OpBranchConditional %cond %20 %99 + + ; inner IfSelection + %20 = OpLabel + %1 = OpCopyObject %uint %uint_1 + OpSelectionMerge %89 None + OpBranchConditional %cond %30 %89 + + %30 = OpLabel ; last block of inner IfSelection + OpBranch %89 + + ; in outer IfSelection + %89 = OpLabel + %3 = OpCopyObject %uint %1; Last use of %1, in outer IfSelection + OpStore %200 %3 + 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(); + + EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(If{ + ( + ScalarConstructor{true} + ) + { + VariableDeclStatement{ + Variable{ + x_1 + none + __u32 + { + ScalarConstructor{1} + } + } + } + If{ + ( + ScalarConstructor{true} + ) + { + } + } + VariableDeclStatement{ + Variable{ + x_3 + none + __u32 + { + Identifier{x_1} + } + } + } + Assignment{ + Identifier{x_200} + Identifier{x_3} + } + } +} +Return{} +)")) << ToString(fe.ast_body()); +} + +TEST_F( + SpvParserTest, + EmitStatement_CombinatorialNonPointer_Hoisting_DefFirstBlockSwitch_InIf) { + // This is like the previous case, but the definition is in a SwitchSelection + // inside another IfSelection. + // Tests that definitions in the first block of a switch count as being + // in the parent of the switch construct. + auto assembly = Preamble() + R"( + %pty = OpTypePointer Private %uint + %200 = OpVariable %pty Private + %cond = OpConstantTrue %bool + + %100 = OpFunction %void None %voidfn + + ; outer IfSelection + %10 = OpLabel + OpSelectionMerge %99 None + OpBranchConditional %cond %20 %99 + + ; inner SwitchSelection + %20 = OpLabel + %1 = OpCopyObject %uint %uint_1 + OpSelectionMerge %89 None + OpSwitch %uint_1 %89 0 %30 + + %30 = OpLabel ; last block of inner SwitchSelection + OpBranch %89 + + ; in outer IfSelection + %89 = OpLabel + %3 = OpCopyObject %uint %1; Last use of %1, in outer IfSelection + OpStore %200 %3 + 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(); + + EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(If{ + ( + ScalarConstructor{true} + ) + { + VariableDeclStatement{ + Variable{ + x_1 + none + __u32 + { + ScalarConstructor{1} + } + } + } + Switch{ + ScalarConstructor{1} + { + Case 0{ + } + Default{ + } + } + } + VariableDeclStatement{ + Variable{ + x_3 + none + __u32 + { + Identifier{x_1} + } + } + } + Assignment{ + Identifier{x_200} + Identifier{x_3} + } + } +} +Return{} +)")) << ToString(fe.ast_body()); +} + TEST_F(SpvParserTest, EmitStatement_Phi_SingleBlockLoopIndex) { auto assembly = Preamble() + R"( %pty = OpTypePointer Private %uint