[PATCH] D12706: Handle non-constant shifts in computeKnownBits, and use computeKnownBits for constant folding in InstCombine/Simplify

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Some comments inline:


================
Comment at: lib/Analysis/ValueTracking.cpp:965
@@ +964,3 @@
+  unsigned BitWidth = KnownZero.getBitWidth();
+
+  if (ConstantInt *SA = dyn_cast<ConstantInt>(I->getOperand(1))) {
----------------
Should we cap `BitWidth` to be some reasonable value (like `128`) so that the loop down below does not run for too long in the worst case?

================
Comment at: lib/Analysis/ValueTracking.cpp:987
@@ +986,3 @@
+
+  bool FirstAllowed = true;
+  for (unsigned ShiftAmt = 0; ShiftAmt < BitWidth; ++ShiftAmt) {
----------------
Why not start `KnownZero` and `KnownOne` as `allOnesValue` and then unconditionally 

```
    KnownZero &= KZF(KnownZero2, ShiftAmt);
    KnownOne  &= KOF(KnownOne2, ShiftAmt);
```

in the loop?

If no shift amount is consistent with `ShiftAmtKZ` and `ShiftAmtKO` then `KnownZero` and `KnownOne` will remain `allOnesValue`; but that's fine since we proved that the shift's result is `undef`, and it's allowed to be `0` and `-1` at the same time.

================
Comment at: lib/Analysis/ValueTracking.cpp:1180
@@ +1179,3 @@
+    auto KZF = [BitWidth](const APInt &KnownZero, unsigned ShiftAmt) {
+      APInt KZ = APIntOps::lshr(KnownZero, ShiftAmt);
+      if (KZ[BitWidth-ShiftAmt-1]) // New bits are known zero.
----------------
Why not just `return APIntOps::ashr(KnownZero, ShiftAmt)`?

================
Comment at: lib/Analysis/ValueTracking.cpp:1188
@@ +1187,3 @@
+      APInt KO = APIntOps::lshr(KnownOne, ShiftAmt);
+      if (KO[BitWidth-ShiftAmt-1]) // // New bits are known one.
+        KO |= APInt::getHighBitsSet(BitWidth, ShiftAmt);
----------------
Why not just `return APIntOps::ashr(KnownOne, ShiftAmt)`?


Repository:
  rL LLVM

http://reviews.llvm.org/D12706