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

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Analysis/ValueTracking.cpp:965
@@ +964,3 @@
+  unsigned BitWidth = KnownZero.getBitWidth();
+
+  if (ConstantInt *SA = dyn_cast<ConstantInt>(I->getOperand(1))) {
----------------
sanjoy wrote:
> 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?
I don't think so. Our canonicalization sometimes creates large integer types, and there's nothing really expensive in the loop (and it is linear with the bit width). If this shows up on a profile somewhere because of large integer sizes, I'll certainly change my mind.


================
Comment at: lib/Analysis/ValueTracking.cpp:983
@@ +982,3 @@
+  if (!(ShiftAmtKZ & (BitWidth-1)) && !(ShiftAmtKO & (BitWidth-1)))
+    return;
+
----------------
jmolloy wrote:
> It seems we could do slightly better here; If the shifter operand is known to be nonzero, we know that we're shifting by at least 1:
> 
>     if (isKnownNonZero(I->getOperand(0), DL, Depth + 1, Q)) {
>       KnownZero = KZF(KnownZero, 1);
>       KnownOne = KOF(KnownOne, 1);
>     }
> 
> I have a similar patch that I was just about to send upstream, but obviously it conflicts with yours and yours handles many more cases.
> 
> Would you be able to put this test in too or should I wait until you've committed this and add it myself?
Either way is fine, but in the name of keeping changes small, it is probably better if you just add it yourself).

================
Comment at: lib/Analysis/ValueTracking.cpp:987
@@ +986,3 @@
+
+  bool FirstAllowed = true;
+  for (unsigned ShiftAmt = 0; ShiftAmt < BitWidth; ++ShiftAmt) {
----------------
sanjoy wrote:
> 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.
We actually can't return non-disjoint bit sets, even for undef, without breaking other code (there are at least two asserts in ValueTracking.cpp that check this explicitly, and I'm afraid of how much else).

That aside, I agree. I'll update the loop to use that form with a check at the end.


================
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.
----------------
sanjoy wrote:
> Why not just `return APIntOps::ashr(KnownZero, ShiftAmt)`?
Good idea (the logic was like this before, but using ashr certainly seems better).

================
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);
----------------
sanjoy wrote:
> Why not just `return APIntOps::ashr(KnownOne, ShiftAmt)`?
Good idea.


http://reviews.llvm.org/D12706