[llvm] 6aa3fc4 - Revert "[InstCombine] propagate 'nsw' on pointer difference of 'inbounds' geps (PR47430)"
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 5 09:25:07 PST 2020
On Thu, Nov 5, 2020 at 2:02 PM Sanjay Patel <spatel at rotateright.com> wrote:
> Nice catch!
> I haven't reviewed all of the subtleties of pointer math (and as
> http://lists.llvm.org/pipermail/llvm-dev/2020-November/146368.html shows,
> there might not be a definite answer yet).
> So do you think the original commit --
> https://reviews.llvm.org/rG324a53205a3af979e3de109fdd52f91781816cba --
> was correct?
>
Yeah, I think your original commit was correct :) It would be in line with
https://reviews.llvm.org/D90708 at least.
Regards,
Nikita
> On Wed, Nov 4, 2020 at 5:51 PM Nikita Popov <nikita.ppv at gmail.com> wrote:
>
>> On Fri, Sep 11, 2020 at 4:55 PM Sanjay Patel via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>>
>>> Author: Sanjay Patel
>>> Date: 2020-09-11T10:54:48-04:00
>>> New Revision: 6aa3fc4a5b88bd0175212e06b183c87cf87c306c
>>>
>>> URL:
>>> https://github.com/llvm/llvm-project/commit/6aa3fc4a5b88bd0175212e06b183c87cf87c306c
>>> DIFF:
>>> https://github.com/llvm/llvm-project/commit/6aa3fc4a5b88bd0175212e06b183c87cf87c306c.diff
>>>
>>> LOG: Revert "[InstCombine] propagate 'nsw' on pointer difference of
>>> 'inbounds' geps (PR47430)"
>>>
>>> This reverts commit 324a53205a3af979e3de109fdd52f91781816cba.
>>>
>>> On closer examination of at least one of the test diffs,
>>> this does not appear to be correct in all cases. Even the
>>> existing 'nsw' creation may be wrong based on this example:
>>> https://alive2.llvm.org/ce/z/uL4Hw9
>>> https://alive2.llvm.org/ce/z/fJMKQS
>>
>>
>> I don't think these proofs are looking at the right thing: Note that the
>> i16 tests are on pointers in address space 1, that specifies a 16-bit
>> pointer and index size in the data layout. If we adjust the data layout in
>> the first proof to p:64:64:64:16, it passes:
>> https://alive2.llvm.org/ce/z/n3vndG
>>
>> Nikita
>>
>> Added:
>>>
>>>
>>> Modified:
>>> llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> llvm/test/Transforms/InstCombine/sub-gep.ll
>>> llvm/test/Transforms/InstCombine/sub.ll
>>>
>>> Removed:
>>>
>>>
>>>
>>>
>>> ################################################################################
>>> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> index a5dd8f6d7c9d..5ce32bc592d0 100644
>>> --- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> +++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>> @@ -1671,12 +1671,11 @@ Value
>>> *InstCombinerImpl::OptimizePointerDifference(Value *LHS, Value *RHS,
>>> I->getOpcode() == Instruction::Mul)
>>> I->setHasNoUnsignedWrap();
>>>
>>> - // If we have a 2nd GEP of the same base pointer, subtract the
>>> offsets.
>>> - // If both GEPs are inbounds, then the subtract does not have signed
>>> overflow.
>>> + // If we had a constant expression GEP on the other side offsetting
>>> the
>>> + // pointer, subtract it from the offset we have.
>>> if (GEP2) {
>>> Value *Offset = EmitGEPOffset(GEP2);
>>> - Result = Builder.CreateSub(Result, Offset, "gep
>>> diff ", /* NUW */ false,
>>> - GEP1->isInBounds() &&
>>> GEP2->isInBounds());
>>> + Result = Builder.CreateSub(Result, Offset, "gep
>>> diff ");
>>> }
>>>
>>> // If we have p - gep(p, ...) then we have to negate the result.
>>>
>>> diff --git a/llvm/test/Transforms/InstCombine/sub-gep.ll
>>> b/llvm/test/Transforms/InstCombine/sub-gep.ll
>>> index ee0c9ffaa0ef..ce9657433bb7 100644
>>> --- a/llvm/test/Transforms/InstCombine/sub-gep.ll
>>> +++ b/llvm/test/Transforms/InstCombine/sub-gep.ll
>>> @@ -245,7 +245,7 @@ define i64 @test24b(i8* %P, i64 %A){
>>> define i64 @test25(i8* %P, i64 %A){
>>> ; CHECK-LABEL: @test25(
>>> ; CHECK-NEXT: [[B_IDX:%.*]] = shl nsw i64 [[A:%.*]], 1
>>> -; CHECK-NEXT: [[GEPDIFF:%.*]] = add nsw i64 [[B_IDX]], -84
>>> +; CHECK-NEXT: [[GEPDIFF:%.*]] = add i64 [[B_IDX]], -84
>>> ; CHECK-NEXT: ret i64 [[GEPDIFF]]
>>> ;
>>> %B = getelementptr inbounds [42 x i16], [42 x i16]* @Arr, i64 0, i64
>>> %A
>>> @@ -260,7 +260,7 @@ define i16 @test25_as1(i8 addrspace(1)* %P, i64 %A) {
>>> ; CHECK-LABEL: @test25_as1(
>>> ; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[A:%.*]] to i16
>>> ; CHECK-NEXT: [[B_IDX:%.*]] = shl nsw i16 [[TMP1]], 1
>>> -; CHECK-NEXT: [[GEPDIFF:%.*]] = add nsw i16 [[B_IDX]], -84
>>> +; CHECK-NEXT: [[GEPDIFF:%.*]] = add i16 [[B_IDX]], -84
>>> ; CHECK-NEXT: ret i16 [[GEPDIFF]]
>>> ;
>>> %B = getelementptr inbounds [42 x i16], [42 x i16] addrspace(1)*
>>> @Arr_as1, i64 0, i64 %A
>>> @@ -272,7 +272,7 @@ define i16 @test25_as1(i8 addrspace(1)* %P, i64 %A) {
>>> define i64 @test30(i8* %foo, i64 %i, i64 %j) {
>>> ; CHECK-LABEL: @test30(
>>> ; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i64 [[I:%.*]], 2
>>> -; CHECK-NEXT: [[GEPDIFF:%.*]] = sub nsw i64 [[GEP1_IDX]], [[J:%.*]]
>>> +; CHECK-NEXT: [[GEPDIFF:%.*]] = sub i64 [[GEP1_IDX]], [[J:%.*]]
>>> ; CHECK-NEXT: ret i64 [[GEPDIFF]]
>>> ;
>>> %bit = bitcast i8* %foo to i32*
>>> @@ -287,7 +287,7 @@ define i64 @test30(i8* %foo, i64 %i, i64 %j) {
>>> define i16 @test30_as1(i8 addrspace(1)* %foo, i16 %i, i16 %j) {
>>> ; CHECK-LABEL: @test30_as1(
>>> ; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i16 [[I:%.*]], 2
>>> -; CHECK-NEXT: [[GEPDIFF:%.*]] = sub nsw i16 [[GEP1_IDX]], [[J:%.*]]
>>> +; CHECK-NEXT: [[GEPDIFF:%.*]] = sub i16 [[GEP1_IDX]], [[J:%.*]]
>>> ; CHECK-NEXT: ret i16 [[GEPDIFF]]
>>> ;
>>> %bit = bitcast i8 addrspace(1)* %foo to i32 addrspace(1)*
>>> @@ -299,11 +299,9 @@ define i16 @test30_as1(i8 addrspace(1)* %foo, i16
>>> %i, i16 %j) {
>>> ret i16 %sub
>>> }
>>>
>>> -; Inbounds translates to 'nsw' on sub
>>> -
>>> define i64 @gep_
>>> diff _both_inbounds(i8* %foo, i64 %i, i64 %j) {
>>> ; CHECK-LABEL: @gep_
>>> diff _both_inbounds(
>>> -; CHECK-NEXT: [[GEPDIFF:%.*]] = sub nsw i64 [[I:%.*]], [[J:%.*]]
>>> +; CHECK-NEXT: [[GEPDIFF:%.*]] = sub i64 [[I:%.*]], [[J:%.*]]
>>> ; CHECK-NEXT: ret i64 [[GEPDIFF]]
>>> ;
>>> %gep1 = getelementptr inbounds i8, i8* %foo, i64 %i
>>> @@ -314,8 +312,6 @@ define i64 @gep_
>>> diff _both_inbounds(i8* %foo, i64 %i, i64 %j) {
>>> ret i64 %sub
>>> }
>>>
>>> -; Negative test for 'nsw' - both geps must be inbounds
>>> -
>>> define i64 @gep_
>>> diff _first_inbounds(i8* %foo, i64 %i, i64 %j) {
>>> ; CHECK-LABEL: @gep_
>>> diff _first_inbounds(
>>> ; CHECK-NEXT: [[GEPDIFF:%.*]] = sub i64 [[I:%.*]], [[J:%.*]]
>>> @@ -329,8 +325,6 @@ define i64 @gep_
>>> diff _first_inbounds(i8* %foo, i64 %i, i64 %j) {
>>> ret i64 %sub
>>> }
>>>
>>> -; Negative test for 'nsw' - both geps must be inbounds
>>> -
>>> define i64 @gep_
>>> diff _second_inbounds(i8* %foo, i64 %i, i64 %j) {
>>> ; CHECK-LABEL: @gep_
>>> diff _second_inbounds(
>>> ; CHECK-NEXT: [[GEPDIFF:%.*]] = sub i64 [[I:%.*]], [[J:%.*]]
>>>
>>> diff --git a/llvm/test/Transforms/InstCombine/sub.ll
>>> b/llvm/test/Transforms/InstCombine/sub.ll
>>> index 0940a08bbb44..98d8a9e6b5ca 100644
>>> --- a/llvm/test/Transforms/InstCombine/sub.ll
>>> +++ b/llvm/test/Transforms/InstCombine/sub.ll
>>> @@ -1077,7 +1077,7 @@ define i64 @test58([100 x [100 x i8]]* %foo, i64
>>> %i, i64 %j) {
>>> ; CHECK-LABEL: @test58(
>>> ; CHECK-NEXT: [[GEP1_OFFS:%.*]] = add i64 [[I:%.*]], 4200
>>> ; CHECK-NEXT: [[GEP2_OFFS:%.*]] = add i64 [[J:%.*]], 4200
>>> -; CHECK-NEXT: [[GEPDIFF:%.*]] = sub nsw i64 [[GEP1_OFFS]],
>>> [[GEP2_OFFS]]
>>> +; CHECK-NEXT: [[GEPDIFF:%.*]] = sub i64 [[GEP1_OFFS]], [[GEP2_OFFS]]
>>> ; CHECK-NEXT: ret i64 [[GEPDIFF]]
>>> ;
>>> %gep1 = getelementptr inbounds [100 x [100 x i8]], [100 x [100 x
>>> i8]]* %foo, i64 0, i64 42, i64 %i
>>>
>>>
>>>
>>> _______________________________________________
>>> 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/20201105/b470d886/attachment.html>
More information about the llvm-commits
mailing list