[PATCH] D38065: [instCombine] Handle (X & C2) < C1 --> (X & C2) == 0

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a subscriber: nlopes.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM. The bit-logic looks right to me - but I miss Alive. :)
cc @nlopes

See inline for some nits.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1720
   unsigned NumTZ = C2->countTrailingZeros();
-  if (Cmp.getPredicate() == ICmpInst::ICMP_UGT && NumTZ < C2->getBitWidth() &&
-      APInt::getOneBitSet(C2->getBitWidth(), NumTZ).ugt(*C1)) {
-    Constant *Zero = Constant::getNullValue(And->getType());
-    return new ICmpInst(ICmpInst::ICMP_NE, And, Zero);
+  if (NumTZ < C2->getBitWidth()) {
+    if (Cmp.getPredicate() == ICmpInst::ICMP_UGT &&
----------------
Is this is a weird way of asking: !C2->isNullValue()  ?


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1722
+    if (Cmp.getPredicate() == ICmpInst::ICMP_UGT &&
+        NumTZ >= C1->getActiveBits()) {
+      Constant *Zero = Constant::getNullValue(And->getType());
----------------
The change to use getActiveBits() could be an NFC commit before this patch.


================
Comment at: test/Transforms/InstCombine/icmp.ll:1141
 
 define i1 @test67inverse(i32 %x) {
 ; CHECK-LABEL: @test67inverse(
----------------
Would be nice to have a test that uses constants at/near the limits. Something like:
define i1 @test67inverse(i8 %x) {
  %and = and i8 %x, -128
  %cmp = icmp ult i8 %and, 126
  ret i1 %cmp
}

Could use vectors to add more testing diversity...assuming that works.


https://reviews.llvm.org/D38065