[PATCH] D12799: [ValueTracking] Teach isKnownNonZero a new trick

[Sanjoy Das via llvm-commits]

sanjoy added a subscriber: sanjoy.

================
Comment at: lib/Analysis/ValueTracking.cpp:1837
@@ +1836,3 @@
+        APInt LoBits = APInt::getLowBitsSet(BitWidth, Shift->getLimitedValue());
+        if ((KnownZero & LoBits).eq(LoBits))
+          return KnownOne.getBoolValue() || isKnownNonZero(X, DL, Depth, Q);
----------------
reames wrote:
> This really seems like two distinct cases and the code would be clearer if organized as such.
> 
> case 1 - There's a known one bit somewhere in the portion not shifted out.
> 
> case 2 - All bits shifted out are known-zero and X is known non-zero.  
> 
> I think what you have is correct, I just found the flow of the code confusing on first read.  
You could also check for the `exact` flag here.

================
Comment at: lib/Analysis/ValueTracking.cpp:1837
@@ +1836,3 @@
+        APInt LoBits = APInt::getLowBitsSet(BitWidth, Shift->getLimitedValue());
+        if ((KnownZero & LoBits).eq(LoBits))
+          return KnownOne.getBoolValue() || isKnownNonZero(X, DL, Depth, Q);
----------------
sanjoy wrote:
> reames wrote:
> > This really seems like two distinct cases and the code would be clearer if organized as such.
> > 
> > case 1 - There's a known one bit somewhere in the portion not shifted out.
> > 
> > case 2 - All bits shifted out are known-zero and X is known non-zero.  
> > 
> > I think what you have is correct, I just found the flow of the code confusing on first read.  
> You could also check for the `exact` flag here.
How about `if (KnownZero.countTrailingZeros() >= Shift->getLimitedValue())`?

================
Comment at: lib/Analysis/ValueTracking.cpp:1838
@@ +1837,3 @@
+        if ((KnownZero & LoBits).eq(LoBits))
+          return KnownOne.getBoolValue() || isKnownNonZero(X, DL, Depth, Q);
+      }
----------------
I'd expect this first case to be handled by the fall through at the end.  Do you have a case where it doesn't?

================
Comment at: unittests/Analysis/ValueTrackingTest.cpp:203
@@ +202,3 @@
+
+TEST_F(KnownValueTest, NonZeroShiftRight) {
+  parseAssembly(
----------------
reames wrote:
> I'd probably just phrase this as a normal IR test for inst combine or inst simplify.  What's here is correct, just needlessly complicated.
+1 to Philip's comment.


Repository:
  rL LLVM

http://reviews.llvm.org/D12799