[PATCH] D23205: [LVI] Make LVI smarter about comparisons with non-constants

[Sanjoy Das via llvm-commits]

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

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1200
@@ +1199,3 @@
+  // of:
+  //  icmp Val, ...
+  //  icmp ult (add Val, Offset), ...
----------------
Do you mean `icmp ult Val, ...`?

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1212
@@ -1211,3 +1220,1 @@
 
-      // If we're interested in the false dest, invert the condition.
-      if (!isTrueDest) TrueValues = TrueValues.inverse();
----------------
Please make this change to how we implement `!isTrueDest` separately.  IMO it _should_ be NFC, but is tricky enough that it should not go in with other changes.

[edit: I understand that *with your change* this isn't NFC, and actually needed for correctness -- so I think this should go in separately as NFC before this change since given the state of LVI today it is NFC.]

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1217
@@ +1216,3 @@
+    // Calculate the range of values that are allowed by the comparison
+    ConstantRange RHSRange(RHS->getType()->getIntegerBitWidth());
+    if (ConstantInt *CI = dyn_cast<ConstantInt>(RHS))
----------------
Can you please explicitly pass in `/*isFullSet=*/true` to the constructor?  The fact that `RHSRange` is the full set is important here.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:1221
@@ -1211,3 +1220,3 @@
 
-      // If we're interested in the false dest, invert the condition.
-      if (!isTrueDest) TrueValues = TrueValues.inverse();
+    CmpInst::Predicate Pred = Predicate;
+    if (!isTrueDest)
----------------
Minor: a ternary operator for `Pred` may be more readable here.


https://reviews.llvm.org/D23205