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

Tue Apr 19 16:56:08 PDT 2022

Breaks the build https://lab.llvm.org/buildbot/#/builders/5/builds/22427

On Tue, 19 Apr 2022 at 13:31, Sanjay Patel via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> 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
>>>
>> _______________________________________________
> 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/03780198/attachment.html>