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

[Roman Lebedev via llvm-commits]

Thank you!

On Wed, Sep 29, 2021 at 6:45 PM Sanjay Patel via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> 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]]
>  ;
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits