[PATCH] D38591: [InstCombine] don't assert that InstSimplify has removed a known true/false cmp (PR34838)

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
Herald added a subscriber: mcrosier.

InstSimplify uses computeKnownBits() to prove that the low bits of a shifted-left value are zeros. Therefore, a cmp with constant with low set bits must be true or false.

In the normal case, it's safe for InstCombine to assert that, but computeKnownBits() bails out when confronted with poison (undef?):

  // If there is conflict between Known.Zero and Known.One, this must be an
  // overflowing left shift, so the shift result is undefined. Clear Known
  // bits so that other code could propagate this undef.

...so I don't think InstCombine can assert as much we'd like. We have to check preconditions at runtime instead.


https://reviews.llvm.org/D38591

Files:
  lib/Transforms/InstCombine/InstCombineCompares.cpp
  test/Transforms/InstCombine/icmp-shl-nsw.ll


Index: test/Transforms/InstCombine/icmp-shl-nsw.ll
===================================================================
--- test/Transforms/InstCombine/icmp-shl-nsw.ll
+++ test/Transforms/InstCombine/icmp-shl-nsw.ll
@@ -354,3 +354,24 @@
   ret i1 %cmp
 }
 
+; https://bugs.llvm.org/show_bug.cgi?id=34838 - the cmp is known true, but
+; the shift is known to create poison, so InstSimplify doesn't kill the cmp.
+; Thus, InstCombine can't assert that the cmp shouldn't exist.
+
+define i1 @PR34838(i8 %x) {
+; CHECK-LABEL: @PR34838(
+; CHECK-NEXT:    [[ZX:%.*]] = zext i8 %x to i16
+; CHECK-NEXT:    [[XOR:%.*]] = xor i16 [[ZX]], 511
+; CHECK-NEXT:    [[SUB:%.*]] = sub nsw i16 [[ZX]], [[XOR]]
+; CHECK-NEXT:    [[SH_MASK:%.*]] = and i16 [[SUB]], 511
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i16 [[SH_MASK]], 0
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+  %zx = zext i8 %x to i16      ; zx  = 0x00xx
+  %xor = xor i16 %zx, 32767    ; xor = 0x7fyy
+  %sub = sub nsw i16 %zx, %xor ; sub = 0x80zz  (the top bit is known one)
+  %sh = shl nsw i16 %sub, 2    ; oops! this shl can't be nsw; that's POISON
+  %cmp = icmp ne i16 %sh, 1
+  ret i1 %cmp
+}
+
Index: lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -1890,12 +1890,13 @@
       return new ICmpInst(Pred, X, ConstantInt::get(ShType, ShiftedC));
     }
     if (Pred == ICmpInst::ICMP_EQ || Pred == ICmpInst::ICMP_NE) {
-      // This is the same code as the SGT case, but assert the pre-condition
-      // that is needed for this to work with equality predicates.
-      assert(C.ashr(*ShiftAmt).shl(*ShiftAmt) == C &&
-             "Compare known true or false was not folded");
-      APInt ShiftedC = C.ashr(*ShiftAmt);
-      return new ICmpInst(Pred, X, ConstantInt::get(ShType, ShiftedC));
+      // This is the same transform as SGT, but we have to avoid the case where
+      // InstSimplify has not removed this compare because the shift is known to
+      // create poison (PR34838).
+      if (C.ashr(*ShiftAmt).shl(*ShiftAmt) == C) {
+        APInt ShiftedC = C.ashr(*ShiftAmt);
+        return new ICmpInst(Pred, X, ConstantInt::get(ShType, ShiftedC));
+      }
     }
     if (Pred == ICmpInst::ICMP_SLT) {
       // SLE is the same as above, but SLE is canonicalized to SLT, so convert:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38591.117862.patch
Type: text/x-patch
Size: 2430 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171005/a1fecab0/attachment.bin>