[llvm] ea56dcb - [InstCombine] fix miscompile from dropRedundantMaskingOfLeftShiftInput()

[Sanjay Patel via llvm-commits]

Author: Sanjay Patel
Date: 2021-09-29T11:43:18-04:00
New Revision: ea56dcb73012f65e6ffb1c320c97c6fbf45480e0

URL: https://github.com/llvm/llvm-project/commit/ea56dcb73012f65e6ffb1c320c97c6fbf45480e0
DIFF: https://github.com/llvm/llvm-project/commit/ea56dcb73012f65e6ffb1c320c97c6fbf45480e0.diff

LOG: [InstCombine] fix miscompile from dropRedundantMaskingOfLeftShiftInput()

The test is from https://llvm.org/PR51351.

There are 2 related logic bugs from over-generalizing "lshr" to "any shr",
but I'm not sure how to expose the difference for "MaskC" because instsimplify
already folds ashr of -1.

I'll extend instsimplify to catch the MaskD pattern as a follow-up, but this
patch should be enough to avoid the miscompile.

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
    llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-d.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
index 8d4113835dc77..cf746a0323f52 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -172,8 +172,8 @@ Value *InstCombinerImpl::reassociateShiftAmtsOfTwoSameDirectionShifts(
 // There are many variants to this pattern:
 //   a)  (x & ((1 << MaskShAmt) - 1)) << ShiftShAmt
 //   b)  (x & (~(-1 << MaskShAmt))) << ShiftShAmt
-//   c)  (x & (-1 >> MaskShAmt)) << ShiftShAmt
-//   d)  (x & ((-1 << MaskShAmt) >> MaskShAmt)) << ShiftShAmt
+//   c)  (x & (-1 l>> MaskShAmt)) << ShiftShAmt
+//   d)  (x & ((-1 << MaskShAmt) l>> MaskShAmt)) << ShiftShAmt
 //   e)  ((x << MaskShAmt) l>> MaskShAmt) << ShiftShAmt
 //   f)  ((x << MaskShAmt) a>> MaskShAmt) << ShiftShAmt
 // All these patterns can be simplified to just:
@@ -213,11 +213,11 @@ dropRedundantMaskingOfLeftShiftInput(BinaryOperator *OuterShift,
   auto MaskA = m_Add(m_Shl(m_One(), m_Value(MaskShAmt)), m_AllOnes());
   // (~(-1 << maskNbits))
   auto MaskB = m_Xor(m_Shl(m_AllOnes(), m_Value(MaskShAmt)), m_AllOnes());
-  // (-1 >> MaskShAmt)
-  auto MaskC = m_Shr(m_AllOnes(), m_Value(MaskShAmt));
-  // ((-1 << MaskShAmt) >> MaskShAmt)
+  // (-1 l>> MaskShAmt)
+  auto MaskC = m_LShr(m_AllOnes(), m_Value(MaskShAmt));
+  // ((-1 << MaskShAmt) l>> MaskShAmt)
   auto MaskD =
-      m_Shr(m_Shl(m_AllOnes(), m_Value(MaskShAmt)), m_Deferred(MaskShAmt));
+      m_LShr(m_Shl(m_AllOnes(), m_Value(MaskShAmt)), m_Deferred(MaskShAmt));
 
   Value *X;
   Constant *NewMask;

diff  --git a/llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-d.ll b/llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-d.ll
index c3039dc84fc52..efc2323c9dd29 100644
--- a/llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-d.ll
+++ b/llvm/test/Transforms/InstCombine/partally-redundant-left-shift-input-masking-after-truncation-variant-d.ll
@@ -246,13 +246,17 @@ define i32 @n6_extrause2(i64 %x, i32 %nbits) {
   ret i32 %t6
 }
 
-; FIXME:
+; TODO: shl+ashr of -1 should be reducecd.
 ; This is a miscompile if it ends by masking off the high bit of the result.
 
 define i32 @PR51351(i64 %x, i32 %nbits) {
 ; CHECK-LABEL: @PR51351(
-; CHECK-NEXT:    [[T3:%.*]] = add i32 [[NBITS:%.*]], -33
-; CHECK-NEXT:    [[T5:%.*]] = trunc i64 [[X:%.*]] to i32
+; CHECK-NEXT:    [[T0:%.*]] = zext i32 [[NBITS:%.*]] to i64
+; CHECK-NEXT:    [[T1:%.*]] = shl i64 -1, [[T0]]
+; CHECK-NEXT:    [[T2:%.*]] = ashr i64 [[T1]], [[T0]]
+; CHECK-NEXT:    [[T3:%.*]] = add i32 [[NBITS]], -33
+; CHECK-NEXT:    [[T4:%.*]] = and i64 [[T2]], [[X:%.*]]
+; CHECK-NEXT:    [[T5:%.*]] = trunc i64 [[T4]] to i32
 ; CHECK-NEXT:    [[T6:%.*]] = shl i32 [[T5]], [[T3]]
 ; CHECK-NEXT:    ret i32 [[T6]]
 ;