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

[spatel]

Summary

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.

[spatel]

I've posted an alternate way to solve this in D38637. If that looks better, I'll abandon this patch.

[spatel]

Abandoning - we can fix this ahead of InstCombine and retain our asserts.

lib/Transforms/InstCombine/InstCombineCompares.cpp

Instruction *InstCombiner::foldICmpShlConstant(ICmpInst &Cmp,
   // the shift.
   if (Shl->hasNoSignedWrap()) {
     if (Pred == ICmpInst::ICMP_SGT) {
       // icmp Pred (shl nsw X, ShiftAmt), C --> icmp Pred X, (C >>s ShiftAmt)
       APInt ShiftedC = C.ashr(*ShiftAmt);
       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");
+      // 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:
       // (X << S) <=s C is equiv to X <=s (C >> S) for all C
       // (X << S) <s (C + 1) is equiv to X <s (C >> S) + 1 if C <s SMAX
       // (X << S) <s C is equiv to X <s ((C - 1) >> S) + 1 if C >s SMIN
       assert(!C.isMinSignedValue() && "Unexpected icmp slt");
       APInt ShiftedC = (C - 1).ashr(*ShiftAmt) + 1;
       return new ICmpInst(Pred, X, ConstantInt::get(ShType, ShiftedC));

test/Transforms/InstCombine/icmp-shl-nsw.ll

 ; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i8 %x, -2
 ; CHECK-NEXT:    ret i1 [[CMP]]
 ;
   %shl = shl nsw i8 %x, 6
   %cmp = icmp ne i8 %shl, -128
   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
+}
+