Fixes in CCP for #1228

* Forces traversal of phis if the def has changed to varying
* Mark a phi as varying if all incoming values are varying
* added a test to catch the bug

diff --git a/source/opt/ccp_pass.cpp b/source/opt/ccp_pass.cpp
index 5852c64..3c85e4f 100644 (file)
--- a/source/opt/ccp_pass.cpp
+++ b/source/opt/ccp_pass.cpp
@@ -62,7 +62,11 @@ SSAPropagator::PropStatus CCPPass::VisitPhi(ir::Instruction* phi) {
     if (it != values_.end()) {
       // We found an argument with a constant value.  Apply the meet operation
       // with the previous arguments.
-      if (meet_val_id == 0) {
+      if (it->second == kVaryingSSAId) {
+        // The "constant" value is actually a placeholder for varying. Return
+        // varying for this phi.
+        return MarkInstructionVarying(phi);
+      } else if (meet_val_id == 0) {
         // This is the first argument we find.  Initialize the result to its
         // constant value id.
         meet_val_id = it->second;

diff --git a/source/opt/propagator.cpp b/source/opt/propagator.cpp
index a046e55..0c085f2 100644 (file)
--- a/source/opt/propagator.cpp
+++ b/source/opt/propagator.cpp
@@ -36,19 +36,19 @@ void SSAPropagator::AddControlEdge(const Edge& edge) {
   blocks_.push(dest_bb);
 }
 
-void SSAPropagator::AddSSAEdges(ir::Instruction* instr) {
+void SSAPropagator::AddSSAEdges(ir::Instruction* instr, bool traverse_phis) {
   // Ignore instructions that produce no result.
   if (instr->result_id() == 0) {
     return;
   }
 
   get_def_use_mgr()->ForEachUser(
-      instr->result_id(), [this](ir::Instruction* use_instr) {
+      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 (use_instr->opcode() == SpvOpPhi) {
+        if (!traverse_phis && use_instr->opcode() == SpvOpPhi) {
           return;
         }
 
@@ -89,8 +89,9 @@ bool SSAPropagator::Simulate(ir::Instruction* instr) {
   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);
+    AddSSAEdges(instr, /* traverse_phis = */ true);
 
     // If |instr| is a block terminator, add all the control edges out of its
     // block.

diff --git a/source/opt/propagator.h b/source/opt/propagator.h
index 69469e0..cb38ca4 100644 (file)
--- a/source/opt/propagator.h
+++ b/source/opt/propagator.h
@@ -259,7 +259,7 @@ class SSAPropagator {
   // 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);
+  void AddSSAEdges(ir::Instruction* instr, bool traverse_phis = false);
 
   // IR context to use.
   ir::IRContext* ctx_;

diff --git a/test/opt/ccp_test.cpp b/test/opt/ccp_test.cpp
index 2e7dabc..3ebf65b 100644 (file)
--- a/test/opt/ccp_test.cpp
+++ b/test/opt/ccp_test.cpp
@@ -581,6 +581,66 @@ TEST_F(CCPTest, SkipSpecConstantInstrucitons) {
   auto res = SinglePassRunToBinary<opt::CCPPass>(spv_asm, true);
   EXPECT_EQ(std::get<1>(res), opt::Pass::Status::SuccessWithoutChange);
 }
+
+TEST_F(CCPTest, UpdateSubsequentPhisToVarying) {
+  const std::string text = R"(
+OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %func "func" %in
+%void = OpTypeVoid
+%bool = OpTypeBool
+%int = OpTypeInt 32 1
+%false = OpConstantFalse %bool
+%int0 = OpConstant %int 0
+%int1 = OpConstant %int 1
+%int6 = OpConstant %int 6
+%int_ptr_Input = OpTypePointer Input %int
+%in = OpVariable %int_ptr_Input Input
+%undef = OpUndef %int
+%functy = OpTypeFunction %void
+%func = OpFunction %void None %functy
+%1 = OpLabel
+OpBranch %2
+%2 = OpLabel
+%outer_phi = OpPhi %int %int0 %1 %outer_add %15
+%cond1 = OpSLessThanEqual %bool %outer_phi %int6
+OpLoopMerge %3 %15 None
+OpBranchConditional %cond1 %4 %3
+%4 = OpLabel
+%ld = OpLoad %int %in
+%cond2 = OpSGreaterThanEqual %bool %int1 %ld
+OpSelectionMerge %10 None
+OpBranchConditional %cond2 %8 %9
+%8 = OpLabel
+OpBranch %10
+%9 = OpLabel
+OpBranch %10
+%10 = OpLabel
+%extra_phi = OpPhi %int %outer_phi %8 %outer_phi %9
+OpBranch %11
+%11 = OpLabel
+%inner_phi = OpPhi %int %int0 %10 %inner_add %13
+%cond3 = OpSLessThanEqual %bool %inner_phi %int6
+OpLoopMerge %14 %13 None
+OpBranchConditional %cond3 %12 %14
+%12 = OpLabel
+OpBranch %13
+%13 = OpLabel
+%inner_add = OpIAdd %int %inner_phi %int1
+OpBranch %11
+%14 = OpLabel
+OpBranch %15
+%15 = OpLabel
+%outer_add = OpIAdd %int %extra_phi %int1
+OpBranch %2
+%3 = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  auto res = SinglePassRunToBinary<opt::CCPPass>(text, true);
+  EXPECT_EQ(std::get<1>(res), opt::Pass::Status::SuccessWithoutChange);
+}
 #endif
 
 }  // namespace