From 716718a5e969f6b4e73cbc864db59a754a83aab3 Mon Sep 17 00:00:00 2001 From: Diego Novillo Date: Fri, 5 Jan 2018 09:34:18 -0500 Subject: [PATCH] Fix infinite simulation cycles in SSA propagator. This fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1159. I had missed a nuance in the original algorithm. When simulating Phi instructions, the SSA edges out of a Phi instruction should never be added to the list of edges to simulate. Phi instructions can be in SSA def-use cycles with other Phi instructions. This was causing the propagator to fall into an infinite loop when the same def-use edge kept being added to the queue. The original algorithm in the paper specifically separates the visit of a Phi instruction vs the visit of a regular instruction. This fix makes the implementation match the original algorithm. --- source/opt/propagator.cpp | 41 ++++++++++++++++++-------------- source/opt/propagator.h | 14 +++++++---- test/opt/ccp_test.cpp | 49 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 22 deletions(-) diff --git a/source/opt/propagator.cpp b/source/opt/propagator.cpp index 10b3a3162..daec5bec1 100644 --- a/source/opt/propagator.cpp +++ b/source/opt/propagator.cpp @@ -36,17 +36,27 @@ void SSAPropagator::AddControlEdge(const Edge& edge) { blocks_.push(dest_bb); } -void SSAPropagator::AddSSAEdges(uint32_t id) { - get_def_use_mgr()->ForEachUser(id, [this](ir::Instruction* instr) { - // If the basic block for |instr| has not been simulated yet, do nothing. - if (!BlockHasBeenSimulated(ctx_->get_instr_block(instr))) { - return; - } +void SSAPropagator::AddSSAEdges(ir::Instruction* instr) { + if (instr->result_id() == 0 || instr->opcode() == SpvOpPhi) { + // Ignore instructions that produce no result and Phi instructions. The SSA + // edges out of Phi instructions are never added. Phi instructions can + // produce cycles in the def-use web and they are always simulated when a + // block is visited. + return; + } - if (ShouldSimulateAgain(instr)) { - ssa_edge_uses_.push(instr); - } - }); + get_def_use_mgr()->ForEachUser( + instr->result_id(), [this](ir::Instruction* use_instr) { + // If the basic block for |use_instr| has not been simulated yet, do + // nothing. + if (!BlockHasBeenSimulated(ctx_->get_instr_block(use_instr))) { + return; + } + + if (ShouldSimulateAgain(use_instr)) { + ssa_edge_uses_.push(use_instr); + } + }); } bool SSAPropagator::IsPhiArgExecutable(ir::Instruction* phi, uint32_t i) const { @@ -74,9 +84,7 @@ bool SSAPropagator::Simulate(ir::Instruction* instr) { // The statement produces a varying result, add it to the list of statements // not to simulate anymore and add its SSA def-use edges for simulation. DontSimulateAgain(instr); - if (instr->result_id() > 0) { - AddSSAEdges(instr->result_id()); - } + AddSSAEdges(instr); // If |instr| is a block terminator, add all the control edges out of its // block. @@ -88,11 +96,8 @@ bool SSAPropagator::Simulate(ir::Instruction* instr) { } return false; } else if (status == kInteresting) { - // If the instruction produced a new interesting value, add the SSA edge - // for its result ID. - if (instr->result_id() > 0) { - AddSSAEdges(instr->result_id()); - } + // Add the SSA edges coming out of this instruction. + AddSSAEdges(instr); // If there are multiple outgoing control flow edges and we know which one // will be taken, add the destination block to the CFG work list. diff --git a/source/opt/propagator.h b/source/opt/propagator.h index 7e7dfc22f..195a304c8 100644 --- a/source/opt/propagator.h +++ b/source/opt/propagator.h @@ -248,12 +248,18 @@ class SSAPropagator { return ctx_->get_def_use_mgr(); } - // If the CFG edge |e| has not been executed, add its destination block to the - // work list. + // If the CFG edge |e| has not been executed, this function adds |e|'s + // destination block to the work list. void AddControlEdge(const Edge& e); - // Add all the instructions that use |id| to the SSA edges work list. - void AddSSAEdges(uint32_t id); + // Adds all the instructions that use the result of |instr| to the SSA edges + // work list. If |instr| produces no result id, this does nothing. This also + // does nothing if |instr| is a Phi instruction. Phi instructions are treated + // specially because (a) they can be in def-use cycles with other + // instructions, and (b) they are always executed when a basic block is + // simulated (see the description of the Sparse Conditional Constant algorithm + // in the original paper). + void AddSSAEdges(ir::Instruction* instr); // IR context to use. ir::IRContext* ctx_; diff --git a/test/opt/ccp_test.cpp b/test/opt/ccp_test.cpp index ecec49dc8..1306c24b2 100644 --- a/test/opt/ccp_test.cpp +++ b/test/opt/ccp_test.cpp @@ -415,6 +415,55 @@ TEST_F(CCPTest, HandleAbortInstructions) { SinglePassRunAndMatch(spv_asm, true); } +TEST_F(CCPTest, SSAWebCycles) { + // Test reduced from https://github.com/KhronosGroup/SPIRV-Tools/issues/1159 + // When there is a cycle in the SSA def-use web, the propagator was getting + // into an infinite loop. SSA edges for Phi instructions should not be + // added to the edges to simulate. + const std::string spv_asm = R"( + OpCapability Shader + %1 = OpExtInstImport "GLSL.std.450" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %main "main" + OpExecutionMode %main OriginUpperLeft + OpSource GLSL 450 + OpName %main "main" + %void = OpTypeVoid + %3 = OpTypeFunction %void + %int = OpTypeInt 32 1 +%_ptr_Function_int = OpTypePointer Function %int + %int_0 = OpConstant %int 0 + %int_4 = OpConstant %int 4 + %bool = OpTypeBool + %int_1 = OpConstant %int 1 +%_ptr_Output_int = OpTypePointer Output %int + %main = OpFunction %void None %3 + %5 = OpLabel + OpBranch %11 + %11 = OpLabel + %29 = OpPhi %int %int_0 %5 %22 %14 + %30 = OpPhi %int %int_0 %5 %25 %14 + OpLoopMerge %13 %14 None + OpBranch %15 + %15 = OpLabel + %19 = OpSLessThan %bool %30 %int_4 +; CHECK: OpBranchConditional %true {{%\d+}} {{%\d+}} + OpBranchConditional %19 %12 %13 + %12 = OpLabel +; CHECK: OpIAdd %int %int_0 %int_0 + %22 = OpIAdd %int %29 %30 + OpBranch %14 + %14 = OpLabel +; CHECK: OpPhi %int %int_0 {{%\d+}} %int_0 {{%\d+}} + %25 = OpPhi %int %int_0 %5 %30 %14 + OpBranch %11 + %13 = OpLabel + OpReturn + OpFunctionEnd + )"; + + SinglePassRunAndMatch(spv_asm, true); +} #endif } // namespace