From 5a75a174d6376fd9b93df1c7e8128b754d1c71ec Mon Sep 17 00:00:00 2001 From: David Neto Date: Mon, 31 Aug 2020 14:05:51 +0000 Subject: [PATCH] [spirv-reader] Uses in phis count as uses This is required so we actually emit values that are only used in phis. Bug: tint:3, tint:215 Change-Id: I9d957a697839b8d09246905c3f28064f0bc01731 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27701 Commit-Queue: David Neto Reviewed-by: dan sinclair --- src/reader/spirv/function.cc | 36 +++--- src/reader/spirv/function_var_test.cc | 167 ++++++++++++++++++++------ 2 files changed, 145 insertions(+), 58 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 724ff47598..f4ae4886dd 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -3244,27 +3244,25 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { const auto* block_info = GetBlockInfo(block_id); const auto block_pos = block_info->pos; for (const auto& inst : *(block_info->basic_block)) { - // 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, 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); + // Update bookkeeping for locally-defined IDs used by this instruction. + inst.ForEachInId([this, block_pos, block_info](const uint32_t* id_ptr) { + auto* def_info = GetDefInfo(*id_ptr); + if (def_info) { + // Update usage count. + def_info->num_uses++; + // Update usage span. + 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; - } + // 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; } - }); - } + } + }); if (inst.opcode() == SpvOpPhi) { // Declare a name for the variable used to carry values to a phi. diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc index ebbac3dda8..d4a46015ba 100644 --- a/src/reader/spirv/function_var_test.cc +++ b/src/reader/spirv/function_var_test.cc @@ -1363,23 +1363,13 @@ TEST_F(SpvParserTest, EmitStatement_Phi_SingleBlockLoopIndex) { } } } - VariableDeclStatement{ - Variable{ - x_4 - none - __u32 - { - Binary{ - Identifier{x_2} - add - ScalarConstructor{1} - } - } - } - } Assignment{ Identifier{x_2_phi} - Identifier{x_4} + Binary{ + Identifier{x_2} + add + ScalarConstructor{1} + } } Assignment{ Identifier{x_3_phi} @@ -1495,6 +1485,13 @@ TEST_F(SpvParserTest, EmitStatement_Phi_MultiBlockLoopIndex) { } } Loop{ + VariableDeclStatement{ + Variable{ + x_4 + function + __u32 + } + } VariableDeclStatement{ Variable{ x_2 @@ -1524,18 +1521,12 @@ TEST_F(SpvParserTest, EmitStatement_Phi_MultiBlockLoopIndex) { } } continuing { - VariableDeclStatement{ - Variable{ - x_4 - none - __u32 - { - Binary{ - Identifier{x_2} - add - ScalarConstructor{1} - } - } + Assignment{ + Identifier{x_4} + Binary{ + Identifier{x_2} + add + ScalarConstructor{1} } } Assignment{ @@ -1632,6 +1623,13 @@ Loop{ ScalarConstructor{1} } Loop{ + VariableDeclStatement{ + Variable{ + x_7 + function + __u32 + } + } VariableDeclStatement{ Variable{ x_2 @@ -1689,18 +1687,12 @@ Loop{ } } continuing { - VariableDeclStatement{ - Variable{ - x_7 - none - __u32 - { - Binary{ - Identifier{x_4} - add - Identifier{x_6} - } - } + Assignment{ + Identifier{x_7} + Binary{ + Identifier{x_4} + add + Identifier{x_6} } } Assignment{ @@ -1957,6 +1949,103 @@ Return{} )")) << ToString(fe.ast_body()); } +TEST_F(SpvParserTest, EmitStatement_UseInPhiCountsAsUse) { + // From crbug.com/215 + // If the only use of a combinatorially computed ID is as the value + // in an OpPhi, then we still have to emit it. The algorithm fix + // is to always count uses in Phis. + // This is the reduced case from the bug report. + // + // The only use of %12 is in the phi. + // The only use of %11 is in %12. + // Both definintions need to be emitted to the output. + auto assembly = Preamble() + R"( + %100 = OpFunction %void None %voidfn + + %10 = OpLabel + %11 = OpLogicalAnd %bool %true %true + %12 = OpLogicalNot %bool %11 ; + OpSelectionMerge %99 None + OpBranchConditional %true %20 %99 + + %20 = OpLabel + OpBranch %99 + + %99 = OpLabel + %101 = OpPhi %bool %11 %10 %12 %20 + 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"(VariableDeclStatement{ + Variable{ + x_101_phi + function + __bool + } +} +VariableDeclStatement{ + Variable{ + x_11 + none + __bool + { + Binary{ + ScalarConstructor{true} + logical_and + ScalarConstructor{true} + } + } + } +} +VariableDeclStatement{ + Variable{ + x_12 + none + __bool + { + UnaryOp{ + not + Identifier{x_11} + } + } + } +} +Assignment{ + Identifier{x_101_phi} + Identifier{x_11} +} +If{ + ( + ScalarConstructor{true} + ) + { + Assignment{ + Identifier{x_101_phi} + Identifier{x_12} + } + } +} +VariableDeclStatement{ + Variable{ + x_101 + none + __bool + { + Identifier{x_101_phi} + } + } +} +Return{} +)")) << ToString(fe.ast_body()); +} + } // namespace } // namespace spirv } // namespace reader