CVP: Improve handling of Selects used as incoming PHI values

[dotdash]

Summary

If the branch that leads to the PHI node and the Select instruction
depend on correlated conditions, we might be able to directly use the
corresponding value from the Select instruction as the incoming value
for the PHI node, allowing later removal of the select instruction.

[dotdash]

Updated the patch to only apply the optimization to selects with a scalar condition.

[dotdash]

FWIW, this helps with an optimization problem we hit with the rust compiler:

https://github.com/rust-lang/rust/issues/24489

[reames]

Seems like a reasonable approach. A couple of small comments, but once those are addressed, should be easy to sign off on.

lib/Transforms/Scalar/CorrelatedValuePropagation.cpp

115

I don't understand why we need the vector type check? Is this a limitation of LVI? If so, it should be pushed inside the LVI function being called. (i.e. returning Unknown for a vector type is perfectly reasonable)

118

I wonder whether a wrapper function on LVI which just take an ICmpInst would be more clear here. The comparison of against true is slightly opaque at first.

test/Transforms/CorrelatedValuePropagation/select.ll

9

A few more tests would be good. In particular:

• a negative test for the vector case
• a simple case without the loop to express the idea
• a negative test for when the select condition *isn't* implied by the branch test.

[dotdash]

lib/Transforms/Scalar/CorrelatedValuePropagation.cpp

118

Actually, I can just use getConstantOnEdge, which is easier to read and better at handling the vector types. Not sure what made me think that I could not use that. With the predicate approach, the false case matches all vectors except for the one with all ones, which is why I had excluded the vector types. I'm currently struggling at coming up with a test for the vector types though.

[dotdash]

Updated the patch to use getConstantOnEdge to improve readability. Added a
simpler test case without a loop and a negative test case. LVI doesn't support
vector types yet, so there's no test for that, but I left a note that support
for vector types may be added later.

[dotdash]

Could somebody have another look at this?

Thanks!

[reames]

LGTM.

lib/Transforms/Scalar/CorrelatedValuePropagation.cpp

bool CorrelatedValuePropagation::processPHI(PHINode *P) {
 
   BasicBlock *BB = P->getParent();
   for (unsigned i = 0, e = P->getNumIncomingValues(); i < e; ++i) {
     Value *Incoming = P->getIncomingValue(i);
     if (isa<Constant>(Incoming)) continue;
 
     Value *V = LVI->getConstantOnEdge(Incoming, P->getIncomingBlock(i), BB, P);
 
-    // Look if the incoming value is a select with a constant but LVI tells us
+    // Look if the incoming value is a select for which LVI can tells us the
+    // condition value, or if its a select with a constant but LVI tells us
     // that the incoming value can never be that constant. In that case replace
-    // the incoming value with the other value of the select. This often allows
-    // us to remove the select later.
+    // the incoming value with the appropriate value of the select. This often
+    // allows us to remove the select later.
     if (!V) {
       SelectInst *SI = dyn_cast<SelectInst>(Incoming);
       if (!SI) continue;
 
+      Value *Condition = SI->getCondition();
+      switch (LVI->getPredicateOnEdge(ICmpInst::ICMP_EQ, Condition,
+                                      ConstantInt::getTrue(Condition->getType()),
+                                      P->getIncomingBlock(i), BB, P)) {
+      case LazyValueInfo::False:
+        V = SI->getFalseValue();
+        break;
+      case LazyValueInfo::True:
+        V = SI->getTrueValue();
+        break;
+      case LazyValueInfo::Unknown:
         Constant *C = dyn_cast<Constant>(SI->getFalseValue());
         if (!C) continue;
 
         if (LVI->getPredicateOnEdge(ICmpInst::ICMP_EQ, SI, C,
-                                  P->getIncomingBlock(i), BB, P) !=
             LazyValueInfo::False)
           continue;
+        V = SI->getTrueValue();
+      }
 
       DEBUG(dbgs() << "CVP: Threading PHI over " << *SI << '\n');
-      V = SI->getTrueValue();
     }
 
     P->setIncomingValue(i, V);
     Changed = true;
   }
 
   // FIXME: Provide TLI, DT, AT to SimplifyInstruction.
   const DataLayout &DL = BB->getModule()->getDataLayout();

test/Transforms/CorrelatedValuePropagation/select.ll

+; RUN: opt < %s -correlated-propagation -S | FileCheck %s
+
+; CHECK-LABEL: @test(
+define void @test(i32) {
+entry:
+  br label %loop
+
+loop:
+  %idx = phi i32 [ %0, %entry ], [ %sel, %loop ]
+; CHECK: %idx = phi i32 [ %0, %entry ], [ %2, %loop ]
+  %1 = icmp eq i32 %idx, 0
+  %2 = add i32 %idx, -1
+  %sel = select i1 %1, i32 0, i32 %2
+  br i1 %1, label %out, label %loop
+
+out:
+  ret void
+}