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

[Chad Rosier via llvm-commits]

mcrosier added a comment.

Thanks for the feedback, Sanjoy.  Let me know if my comments make sense.  I'll upload a new diff shortly.


================
Comment at: lib/Analysis/ValueTracking.cpp:3823
@@ +3822,3 @@
+  // equivalent to reduce the amount of logic needed.
+  APred = ICmpInst::getUnsignedPredicate(APred);
+  BPred = ICmpInst::getUnsignedPredicate(BPred);
----------------
sanjoy wrote:
> This scares me.  Can you use a different variable name for `APred` and `BPred` here, like `UnsignedAPred`?
Sure, no problem.  I don't want to scare anyone. :)

================
Comment at: lib/Analysis/ValueTracking.cpp:3850
@@ +3849,3 @@
+    if (BPred == CmpInst::ICMP_EQ ||  // A > B implies A == B is false.
+        BPred == CmpInst::ICMP_ULE) { // A > B implies A <= B is false.
+      ImpliedTrue = false;
----------------
sanjoy wrote:
> Won't this return true for `A ugt B => NOT(A sle B)` (since you unconditionally coerced both to the unsigned variants) (consider A = -1, B = 0)?
See my comment below about checking to make sure the predicates share the same sign, if they're signed.  I believe such a check would avoid this situation.

================
Comment at: lib/Analysis/ValueTracking.cpp:3909
@@ -3806,2 +3908,3 @@
     return false;
 
+  if (isImpliedCondMatchingOperands(APred, ALHS, ARHS, BPred, BLHS, BRHS,
----------------
At minimum, I'm thinking we need a check here to make sure both compares have the same sign (or at least one compare is an equality check, which has no sign).

We don't think we want 'icmp ugt a, b' to imply 'icmp sgt b, a' is false, for example.

I'll add tests cases.


http://reviews.llvm.org/D18905