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.

diff --git a/source/opt/propagator.cpp b/source/opt/propagator.cpp
index 10b3a31..daec5be 100644 (file)
--- 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 7e7dfc2..195a304 100644 (file)
--- 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 ecec49d..1306c24 100644 (file)
--- a/test/opt/ccp_test.cpp
+++ b/test/opt/ccp_test.cpp
@@ -415,6 +415,55 @@ TEST_F(CCPTest, HandleAbortInstructions) {
   SinglePassRunAndMatch<opt::CCPPass>(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<opt::CCPPass>(spv_asm, true);
+}
 #endif
 
 }  // namespace