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

[Sanjoy Das via llvm-commits]

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

================
Comment at: include/llvm/Analysis/ValueTracking.h:433
@@ +432,3 @@
+  /// Return true if RHS is known to be implied by LHS.  The implication may be
+  /// either true or false depending on the value of ImpliedTrue.
+  /// A & B must be i1 (boolean) values or a vector of such values. Note that
----------------
mcrosier wrote:
> I've updated everything to ImpliedTrue.  Mind if I defer the second suggestion? :)
SGTM.

But I'd change the doxygen comment slightly -- 'depending on the value of ImpliedTrue' -> 'depending on what is returned in ImpliedTrue', to make it obvious that ImpliedTrue is a return parameter.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2134
@@ -2133,3 +2133,3 @@
       break;
-    case ICmpInst::ICMP_UGE:
+    case ICmpInst::ICMP_UGE: {
       // X >=u 1 -> X
----------------
I didn't notice these last time, but I'm not sure if this usage is correct (or will remain correct, even if they're correct in practice after this patch :)): `isImpliedCondition` will return true with `ImpliedTrue` set to false if `RHS -> NOT(LHS)`.  But in that case you're simplifying `RHS -> LHS` to `false`, meaning you're asserting `NOT(RHS -> LHS)`.  `NOT(RHS -> LHS)` is not the same as `RHS -> NOT(LHS)` if `RHS` is `false`.


http://reviews.llvm.org/D18905