[llvm] 8a9c70f - [InstCombine] C0 shift (X add nuw C) --> (C0 shift C) shift X

[Sanjay Patel via llvm-commits]

Good point! I removed those calls, and the existing tests for the fold
(just above the ones that I added) all still pass.

The existing code was already strange to be calling isKnownNonNegative on a
constant.
I'll let this bake a bit then remove that clause and update the comment.

On Tue, Apr 19, 2022 at 3:30 PM Nikita Popov <nikita.ppv at gmail.com> wrote:

> On Tue, Apr 19, 2022 at 9:21 PM Sanjay Patel via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> Author: Sanjay Patel
>> Date: 2022-04-19T15:21:34-04:00
>> New Revision: 8a9c70fc01e6c900f060f3c23d96ee444be33a9a
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/8a9c70fc01e6c900f060f3c23d96ee444be33a9a
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/8a9c70fc01e6c900f060f3c23d96ee444be33a9a.diff
>>
>> LOG: [InstCombine] C0 shift (X add nuw C) --> (C0 shift C) shift X
>>
>> With 'nuw' we can convert the increment of the shift amount
>> into a pre-shift (constant fold) of the shifted constant:
>> https://alive2.llvm.org/ce/z/FkTyR2
>>
>> Fixes issue #41976
>>
>> Added:
>>
>>
>> Modified:
>>     llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
>>     llvm/test/Transforms/InstCombine/shift-add.ll
>>
>> Removed:
>>
>>
>>
>>
>> ################################################################################
>> diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
>> b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
>> index 26fe8e08d066d..cc4d53d27f031 100644
>> --- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
>> +++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
>> @@ -399,13 +399,14 @@ Instruction
>> *InstCombinerImpl::commonShiftTransforms(BinaryOperator &I) {
>>            reassociateShiftAmtsOfTwoSameDirectionShifts(&I, SQ)))
>>      return NewShift;
>>
>> -  // (C1 shift (A add C2)) -> (C1 shift C2) shift A)
>> -  // iff A and C2 are both positive.
>> +  // C0 shift (A add C) -> (C0 shift C) shift A
>> +  // iff A and C2 are both positive or the add has 'nuw'.
>>    Value *A;
>>    Constant *C;
>>    if (match(Op0, m_Constant()) && match(Op1, m_Add(m_Value(A),
>> m_Constant(C))))
>> -    if (isKnownNonNegative(A, DL, 0, &AC, &I, &DT) &&
>> -        isKnownNonNegative(C, DL, 0, &AC, &I, &DT))
>> +    if (cast<BinaryOperator>(Op1)->hasNoUnsignedWrap() ||
>> +        (isKnownNonNegative(A, DL, 0, &AC, &I, &DT) &&
>> +         isKnownNonNegative(C, DL, 0, &AC, &I, &DT)))
>>        return BinaryOperator::Create(
>>            I.getOpcode(), Builder.CreateBinOp(I.getOpcode(), Op0, C), A);
>>
>
> Just wondering, are those isKnownNonNegative() actually still necessary?
> I'd assume that we'd infer "add nuw" from those anyway.
>
> Nikita
>
>
>>
>> diff  --git a/llvm/test/Transforms/InstCombine/shift-add.ll
>> b/llvm/test/Transforms/InstCombine/shift-add.ll
>> index d9a1c3b1878b3..f75f6a95f8f8a 100644
>> --- a/llvm/test/Transforms/InstCombine/shift-add.ll
>> +++ b/llvm/test/Transforms/InstCombine/shift-add.ll
>> @@ -123,8 +123,7 @@ define <4 x i32> @lshr_C1_add_A_C2_v4i32_splat(i16
>> %I) {
>>
>>  define i32 @shl_add_nuw(i32 %x) {
>>  ; CHECK-LABEL: @shl_add_nuw(
>> -; CHECK-NEXT:    [[A:%.*]] = add nuw i32 [[X:%.*]], 5
>> -; CHECK-NEXT:    [[R:%.*]] = shl i32 6, [[A]]
>> +; CHECK-NEXT:    [[R:%.*]] = shl i32 192, [[X:%.*]]
>>  ; CHECK-NEXT:    ret i32 [[R]]
>>  ;
>>    %a = add nuw i32 %x, 5
>> @@ -132,10 +131,11 @@ define i32 @shl_add_nuw(i32 %x) {
>>    ret i32 %r
>>  }
>>
>> +; vectors with arbitrary constants work too
>> +
>>  define <2 x i12> @lshr_add_nuw(<2 x i12> %x) {
>>  ; CHECK-LABEL: @lshr_add_nuw(
>> -; CHECK-NEXT:    [[A:%.*]] = add nuw <2 x i12> [[X:%.*]], <i12 5, i12 1>
>> -; CHECK-NEXT:    [[R:%.*]] = lshr <2 x i12> <i12 6, i12 42>, [[A]]
>> +; CHECK-NEXT:    [[R:%.*]] = lshr <2 x i12> <i12 0, i12 21>, [[X:%.*]]
>>  ; CHECK-NEXT:    ret <2 x i12> [[R]]
>>  ;
>>    %a = add nuw <2 x i12> %x, <i12 5, i12 1>
>> @@ -143,12 +143,13 @@ define <2 x i12> @lshr_add_nuw(<2 x i12> %x) {
>>    ret <2 x i12> %r
>>  }
>>
>> +; extra use is ok and in this case the result can be simplified to a
>> constant
>> +
>>  define i32 @ashr_add_nuw(i32 %x, i32* %p) {
>>  ; CHECK-LABEL: @ashr_add_nuw(
>>  ; CHECK-NEXT:    [[A:%.*]] = add nuw i32 [[X:%.*]], 5
>>  ; CHECK-NEXT:    store i32 [[A]], i32* [[P:%.*]], align 4
>> -; CHECK-NEXT:    [[R:%.*]] = ashr i32 -6, [[A]]
>> -; CHECK-NEXT:    ret i32 [[R]]
>> +; CHECK-NEXT:    ret i32 -1
>>  ;
>>    %a = add nuw i32 %x, 5
>>    store i32 %a, i32* %p
>> @@ -156,6 +157,8 @@ define i32 @ashr_add_nuw(i32 %x, i32* %p) {
>>    ret i32 %r
>>  }
>>
>> +; negative test - must have 'nuw'
>> +
>>  define i32 @shl_add_nsw(i32 %x) {
>>  ; CHECK-LABEL: @shl_add_nsw(
>>  ; CHECK-NEXT:    [[A:%.*]] = add nsw i32 [[X:%.*]], 5
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220419/bf11a695/attachment.html>