[PATCH] D18905: [ValueTracking] Improve isImpliedCondition for conditions with matching operands.

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Analysis/ValueTracking.cpp:3801
@@ +3800,3 @@
+
+/// Return true if "icmp2 Pred op1 op2" is known to be implied by "icmp1 Pred
+/// op1 op2".  The implication may be either true or false depending on the
----------------
I'd change the comment to be in terms of the formal parameters (i.e. `APred`, `ARHS` etc.) to make things more obvious.

================
Comment at: lib/Analysis/ValueTracking.cpp:3832
@@ +3831,3 @@
+  assert(PredicatesComparable(APred, BPred) && "Predicates can't be compared!");
+  CmpInst::Predicate UnsignedAPred = ICmpInst::getUnsignedPredicate(APred);
+  CmpInst::Predicate UnsignedBPred = ICmpInst::getUnsignedPredicate(BPred);
----------------
Nit: I'd use `auto` here.

================
Comment at: lib/Analysis/ValueTracking.cpp:3834
@@ +3833,3 @@
+  CmpInst::Predicate UnsignedBPred = ICmpInst::getUnsignedPredicate(BPred);
+  switch (UnsignedAPred) {
+  default:
----------------
This does not need to happen in this change, but it really looks like this logic is general enough to live in `CmpInst` along with things like `isTrueWhenEqual` as `CmpInst::isImpliedPredicate`.

================
Comment at: lib/Analysis/ValueTracking.cpp:3838
@@ +3837,3 @@
+  case CmpInst::ICMP_EQ:
+    if (UnsignedBPred == CmpInst::ICMP_UGT || // A == B implies A > B is false
+        UnsignedBPred == CmpInst::ICMP_ULT || // A == B implies A < B is false
----------------
Can't you use `CmpInst::isFalseWhenEqual` here?

================
Comment at: lib/Analysis/ValueTracking.cpp:3876
@@ +3875,3 @@
+  case CmpInst::ICMP_UGE:
+    if (UnsignedBPred == CmpInst::ICMP_ULT) { // A >= B implies A < B is false.
+      ImpliedTrue = false;
----------------
Some of this boilerplate can be removed by checking if `getInversePredicate(APred)` == `BPred` outside the switch.

================
Comment at: lib/Analysis/ValueTracking.cpp:3900-3903
@@ -3799,3 +3899,6 @@
   // LHS ==> RHS by definition
-  if (LHS == RHS) return true;
+  if (LHS == RHS) {
+    ImpliedTrue = true;
+    return true;
+  }
 
----------------
This is why I'd have liked an `Optional<bool>` return value. :)  Avoids mistakes like these.

================
Comment at: lib/Analysis/ValueTracking.cpp:3919
@@ +3918,3 @@
+  // The predicates must match sign or at least one of them must be an equality
+  // comparison (which is signless).
+  if (!PredicatesComparable(APred, BPred))
----------------
I wouldn't do the check here, since it isn't obvious how this check relates to `isImpliedCondMatchingOperands` or even why the precondition is needed.  Instead, I'll change the `assert` in `isImpliedCondMatchingOperands` to return false if the predicates are not comparable.


http://reviews.llvm.org/D18905