[PATCH] D38637: [InstSimplify] don't let poison inhibit an easy fold

[Sanjay Patel via Phabricator via llvm-commits]

spatel updated this revision to Diff 118139.
spatel added a comment.

Patch updated:
Fix bogus comment about undef and add a TODO for a potential follow-up patch.


https://reviews.llvm.org/D38637

Files:
  lib/Analysis/ValueTracking.cpp
  test/Transforms/InstSimplify/icmp-constant.ll


Index: test/Transforms/InstSimplify/icmp-constant.ll
===================================================================
--- test/Transforms/InstSimplify/icmp-constant.ll
+++ test/Transforms/InstSimplify/icmp-constant.ll
@@ -576,12 +576,7 @@
 
 define i1 @ne_shl_by_constant_produces_poison(i8 %x) {
 ; CHECK-LABEL: @ne_shl_by_constant_produces_poison(
-; CHECK-NEXT:    [[ZX:%.*]] = zext i8 %x to i16
-; CHECK-NEXT:    [[XOR:%.*]] = xor i16 [[ZX]], 32767
-; CHECK-NEXT:    [[SUB:%.*]] = sub nsw i16 [[ZX]], [[XOR]]
-; CHECK-NEXT:    [[POISON:%.*]] = shl nsw i16 [[SUB]], 2
-; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i16 [[POISON]], 1
-; CHECK-NEXT:    ret i1 [[CMP]]
+; CHECK-NEXT:    ret i1 true
 ;
   %zx = zext i8 %x to i16      ; zx  = 0x00xx
   %xor = xor i16 %zx, 32767    ; xor = 0x7fyy
@@ -593,11 +588,7 @@
 
 define i1 @eq_shl_by_constant_produces_poison(i8 %x) {
 ; CHECK-LABEL: @eq_shl_by_constant_produces_poison(
-; CHECK-NEXT:    [[CLEAR_HIGH_BIT:%.*]] = and i8 %x, 127
-; CHECK-NEXT:    [[SET_NEXT_HIGH_BITS:%.*]] = or i8 [[CLEAR_HIGH_BIT]], 112
-; CHECK-NEXT:    [[POISON:%.*]] = shl nsw i8 [[SET_NEXT_HIGH_BITS]], 3
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[POISON]], 15
-; CHECK-NEXT:    ret i1 [[CMP]]
+; CHECK-NEXT:    ret i1 false
 ;
   %clear_high_bit = and i8 %x, 127                 ; 0x7f
   %set_next_high_bits = or i8 %clear_high_bit, 112 ; 0x70
@@ -612,13 +603,7 @@
 
 define i1 @eq_shl_by_variable_produces_poison(i8 %x) {
 ; CHECK-LABEL: @eq_shl_by_variable_produces_poison(
-; CHECK-NEXT:    [[CLEAR_HIGH_BIT:%.*]] = and i8 %x, 127
-; CHECK-NEXT:    [[SET_NEXT_HIGH_BITS:%.*]] = or i8 [[CLEAR_HIGH_BIT]], 112
-; CHECK-NEXT:    [[NOTUNDEF_SHIFTAMT:%.*]] = and i8 %x, 3
-; CHECK-NEXT:    [[NONZERO_SHIFTAMT:%.*]] = or i8 [[NOTUNDEF_SHIFTAMT]], 1
-; CHECK-NEXT:    [[POISON:%.*]] = shl nsw i8 [[SET_NEXT_HIGH_BITS]], [[NONZERO_SHIFTAMT]]
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8 [[POISON]], 15
-; CHECK-NEXT:    ret i1 [[CMP]]
+; CHECK-NEXT:    ret i1 false
 ;
   %clear_high_bit = and i8 %x, 127                 ; 0x7f
   %set_next_high_bits = or i8 %clear_high_bit, 112 ; 0x70
Index: lib/Analysis/ValueTracking.cpp
===================================================================
--- lib/Analysis/ValueTracking.cpp
+++ lib/Analysis/ValueTracking.cpp
@@ -808,19 +808,20 @@
     computeKnownBits(I->getOperand(0), Known, Depth + 1, Q);
     Known.Zero = KZF(Known.Zero, ShiftAmt);
     Known.One  = KOF(Known.One, ShiftAmt);
-    // 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.
-    if ((Known.Zero & Known.One) != 0)
-      Known.resetAll();
+    // If the known bits conflict, this must be an overflowing left shift, so
+    // the shift result is poison. We can return anything we want. Choose 0 for
+    // the best folding opportunity.
+    if (Known.hasConflict())
+      Known.setAllZero();
 
     return;
   }
 
   computeKnownBits(I->getOperand(1), Known, Depth + 1, Q);
 
-  // If the shift amount could be greater than or equal to the bit-width of the LHS, the
-  // value could be undef, so we don't know anything about it.
+  // If the shift amount could be greater than or equal to the bit-width of the
+  // LHS, the value could be poison, but bail out because the check below is
+  // expensive. TODO: Should we just carry on?
   if ((~Known.Zero).uge(BitWidth)) {
     Known.resetAll();
     return;
@@ -876,13 +877,10 @@
     Known.One  &= KOF(Known2.One, ShiftAmt);
   }
 
-  // If there are no compatible shift amounts, then we've proven that the shift
-  // amount must be >= the BitWidth, and the result is undefined. We could
-  // return anything we'd like, but we need to make sure the sets of known bits
-  // stay disjoint (it should be better for some other code to actually
-  // propagate the undef than to pick a value here using known bits).
-  if (Known.Zero.intersects(Known.One))
-    Known.resetAll();
+  // If the known bits conflict, the result is poison. Return a 0 and hope the
+  // caller can further optimize that.
+  if (Known.hasConflict())
+    Known.setAllZero();
 }
 
 static void computeKnownBitsFromOperator(const Operator *I, KnownBits &Known,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38637.118139.patch
Type: text/x-patch
Size: 4293 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171007/1400453f/attachment.bin>