Fixes #1300. Adding checks for bad CCP transitions and unsettled values

* Now track propagation status and assert on bad statuses
 * Added helper methods to access instruction propagation status
* Modified the phi meet operator to properly reflect the paper it is
based on
* Modified SSA edge addition so that all edge are added, but only on
state changes
* Fixed a bug in instruction simulation where interesting conditional
branches would not mark the interesting edge as executed
 * Added a test to catch this bug
* Added an ostream operator for SSAPropagator::PropStatus

source/opt/ccp_pass.cpp

@@ -81,9 +81,10 @@ SSAPropagator::PropStatus CCPPass::VisitPhi(ir::Instruction* phi) {
         return MarkInstructionVarying(phi);
       }
     } else {
-      // If any argument is not a constant, the Phi produces nothing
-      // interesting for now. The propagator will callback again, if needed.
-      return SSAPropagator::kNotInteresting;
+      // The incoming value has no recorded value and is therefore not
+      // interesting. A not interesting value joined with any other value is the
+      // other value.
+      continue;
     }
   }
 
@@ -258,6 +259,7 @@ bool CCPPass::PropagateConstants(ir::Function* fp) {
 
   propagator_ =
       std::unique_ptr<SSAPropagator>(new SSAPropagator(context(), visit_fn));
+
   if (propagator_->Run(fp)) {
     return ReplaceValues();
   }

source/opt/propagator.cpp

@@ -36,22 +36,14 @@ void SSAPropagator::AddControlEdge(const Edge& edge) {
   blocks_.push(dest_bb);
 }
 
-void SSAPropagator::AddSSAEdges(ir::Instruction* instr, bool traverse_phis) {
+void SSAPropagator::AddSSAEdges(ir::Instruction* instr) {
   // Ignore instructions that produce no result.
   if (instr->result_id() == 0) {
     return;
   }
 
   get_def_use_mgr()->ForEachUser(
-      instr->result_id(), [this, traverse_phis](ir::Instruction* use_instr) {
-        // If |use_instr| is a Phi, ignore this edge.  Phi instructions can form
-        // cycles in the def-use web, which would get the propagator into an
-        // infinite loop.  Phi instructions are always simulated when a block is
-        // visited, so there is no need to traverse the SSA edges into them.
-        if (!traverse_phis && use_instr->opcode() == SpvOpPhi) {
-          return;
-        }
-
+      instr->result_id(), [this](ir::Instruction* use_instr) {
         // If the basic block for |use_instr| has not been simulated yet, do
         // nothing.  The instruction |use_instr| will be simulated next time the
         // block is scheduled.
@@ -75,6 +67,23 @@ bool SSAPropagator::IsPhiArgExecutable(ir::Instruction* phi, uint32_t i) const {
   return IsEdgeExecutable(Edge(in_bb, phi_bb));
 }
 
+bool SSAPropagator::SetStatus(ir::Instruction* inst, PropStatus status) {
+  bool has_old_status = false;
+  PropStatus old_status = kVarying;
+  if (HasStatus(inst)) {
+    has_old_status = true;
+    old_status = Status(inst);
+  }
+
+  assert((!has_old_status || old_status <= status) &&
+         "Invalid lattice transition");
+
+  bool status_changed = !has_old_status || (old_status != status);
+  if (status_changed) statuses_[inst] = status;
+
+  return status_changed;
+}
+
 bool SSAPropagator::Simulate(ir::Instruction* instr) {
   bool changed = false;
 
@@ -85,13 +94,15 @@ bool SSAPropagator::Simulate(ir::Instruction* instr) {
 
   ir::BasicBlock* dest_bb = nullptr;
   PropStatus status = visit_fn_(instr, &dest_bb);
+  bool status_changed = SetStatus(instr, status);
 
   if (status == kVarying) {
     // 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.
-    // Force re-simulation of all uses of this instruction.
     DontSimulateAgain(instr);
-    AddSSAEdges(instr, /* traverse_phis = */ true);
+    if (status_changed) {
+      AddSSAEdges(instr);
+    }
 
     // If |instr| is a block terminator, add all the control edges out of its
     // block.
@@ -103,13 +114,16 @@ bool SSAPropagator::Simulate(ir::Instruction* instr) {
     }
     return false;
   } else if (status == kInteresting) {
-    // Add the SSA edges coming out of this instruction.
-    AddSSAEdges(instr);
+    // Add the SSA edges coming out of this instruction if the propagation
+    // status has changed.
+    if (status_changed) {
+      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.
     if (dest_bb) {
-      blocks_.push(dest_bb);
+      AddControlEdge(Edge(ctx_->get_instr_block(instr), dest_bb));
     }
     changed = true;
   }
@@ -244,8 +258,34 @@ bool SSAPropagator::Run(ir::Function* fn) {
     }
   }
 
+#ifndef NDEBUG
+  // Verify all visited values have settled. No value that has been simulated
+  // should end on not interesting.
+  fn->ForEachInst([this](ir::Instruction* inst) {
+    assert(
+        (!HasStatus(inst) || Status(inst) != SSAPropagator::kNotInteresting) &&
+        "Unsettled value");
+  });
+#endif
+
   return changed;
 }
 
+std::ostream& operator<<(std::ostream& str,
+                         const SSAPropagator::PropStatus& status) {
+  switch (status) {
+    case SSAPropagator::kVarying:
+      str << "Varying";
+      break;
+    case SSAPropagator::kInteresting:
+      str << "Interesting";
+      break;
+    default:
+      str << "Not interesting";
+      break;
+  }
+  return str;
+}
+
 }  // namespace opt
 }  // namespace spvtools

source/opt/propagator.h

@@ -198,6 +198,20 @@ class SSAPropagator {
   // Instruction::GetSingleWordOperand.
   bool IsPhiArgExecutable(ir::Instruction* phi, uint32_t i) const;
 
+  // Returns true if |inst| has a recorded status. This will be true once |inst|
+  // has been simulated once.
+  bool HasStatus(ir::Instruction* inst) const { return statuses_.count(inst); }
+
+  // Returns the current propagation status of |inst|. Assumes
+  // |HasStatus(inst)| returns true.
+  PropStatus Status(ir::Instruction* inst) const {
+    return statuses_.find(inst)->second;
+  }
+
+  // Records the propagation status |status| for |inst|. Returns true if the
+  // status for |inst| has changed or set was set for the first time.
+  bool SetStatus(ir::Instruction* inst, PropStatus status);
+
  private:
   // Initialize processing.
   void Initialize(ir::Function* fn);
@@ -253,13 +267,8 @@ class SSAPropagator {
   void AddControlEdge(const Edge& e);
 
   // 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 the instruction at the end of the def-use is a Phi
-  // instruction.  Phi instructions are treated specially because (a) they can
-  // be in def-use cycles with other Phi 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, bool traverse_phis = false);
+  // work list. If |instr| produces no result id, this does nothing.
+  void AddSSAEdges(ir::Instruction* instr);
 
   // IR context to use.
   ir::IRContext* ctx_;
@@ -296,8 +305,14 @@ class SSAPropagator {
 
   // Set of executable CFG edges.
   std::set<Edge> executable_edges_;
+
+  // Tracks instruction propagation status.
+  std::unordered_map<ir::Instruction*, SSAPropagator::PropStatus> statuses_;
 };
 
+std::ostream& operator<<(std::ostream& str,
+                         const SSAPropagator::PropStatus& status);
+
 }  // namespace opt
 }  // namespace spvtools
 

test/opt/ccp_test.cpp

@@ -458,14 +458,15 @@ TEST_F(CCPTest, SSAWebCycles) {
          %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
+; CHECK: OpPhi %int %int_0 {{%\d+}}
+         %25 = OpPhi %int %30 %12
                OpBranch %11
          %13 = OpLabel
                OpReturn
                OpFunctionEnd
   )";
 
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
   SinglePassRunAndMatch<opt::CCPPass>(spv_asm, true);
 }
 
@@ -705,6 +706,33 @@ TEST_F(CCPTest, UseConstantFoldingRules) {
 
   SinglePassRunAndMatch<opt::CCPPass>(text, true);
 }
+
+// Test for #1300. Previously value for %5 would not settle during simulation.
+TEST_F(CCPTest, SettlePhiLatticeValue) {
+  const std::string text = R"(
+OpCapability Kernel
+OpCapability Linkage
+OpMemoryModel Logical OpenCL
+OpDecorate %func LinkageAttributes "func" Export
+%void = OpTypeVoid
+%bool = OpTypeBool
+%true = OpConstantTrue %bool
+%false = OpConstantFalse %bool
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+OpBranchConditional %true %2 %3
+%3 = OpLabel
+OpBranch %2
+%2 = OpLabel
+%5 = OpPhi %bool %true %1 %false %3
+OpReturn
+OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  SinglePassRunToBinary<opt::CCPPass>(text, true);
+}
 #endif
 
 }  // namespace