[PATCH] D18777: [ValueTracking] An improvement to IR ValueTracking on Non-negative Integers

[Sanjoy Das via llvm-commits]

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

================
Comment at: lib/Analysis/ValueTracking.cpp:788
@@ +787,3 @@
+                                         APInt &KnownZero, APInt &KnownOne,
+                                         APInt &KnownZero2, APInt &KnownOne2) {
+  // If this is a left shift with nsw, then the shift produces a poison value
----------------
I'd s/`KnownZero2`/`KnownZeroOp` (same for `KnownOne2`), and s/`KnownZero`/`KnownZeroResult`/.  Also, pass in `KnownZeroOp` by const ref.

================
Comment at: lib/Analysis/ValueTracking.cpp:796
@@ +795,3 @@
+      cast<OverflowingBinaryOperator>(I)->hasNoSignedWrap()) {
+    if (KnownZero2.isNegative())
+        KnownZero |= APInt::getSignBit(BitWidth);
----------------
Why can't this be folded into the `KZF` and `KOF` for the `Instruction::Shl` case?

================
Comment at: lib/Analysis/ValueTracking.cpp:1271
@@ +1270,3 @@
+
+          auto OverflowOp = dyn_cast<OverflowingBinaryOperator>(LU);
+          if (OverflowOp && OverflowOp->hasNoSignedWrap()) {
----------------
Use `auto *`.

================
Comment at: lib/Analysis/ValueTracking.cpp:1288
@@ +1287,3 @@
+            }
+            
+            // (sub nsw non-negative, negative) --> non-negative
----------------
What does this bit have to do with the changes to how we handle shifts?  If they're not related, they should definitely be reviewed/tested/committed separately.


https://reviews.llvm.org/D18777