[PATCH] CVP: Improve handling of Selects used as incoming PHI values

[Philip Reames]

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


================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:115
@@ +114,3 @@
+      Value *Condition = SI->getCondition();
+      if (!Condition->getType()->isVectorTy()) {
+        switch (LVI->getPredicateOnEdge(ICmpInst::ICMP_EQ, Condition,
----------------
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)

================
Comment at: lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:118
@@ +117,3 @@
+              ConstantInt::getTrue(Condition->getType()),
+              P->getIncomingBlock(i), BB, P)) {
+          case LazyValueInfo::True:
----------------
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.  

================
Comment at: test/Transforms/CorrelatedValuePropagation/select.ll:8
@@ +7,3 @@
+
+loop:
+  %idx = phi i32 [ %0, %entry ], [ %sel, %loop ]
----------------
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.

http://reviews.llvm.org/D9051

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/