[llvm] 6aa3fc4 - Revert "[InstCombine] propagate 'nsw' on pointer difference of 'inbounds' geps (PR47430)"
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 5 05:01:48 PST 2020
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?
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/b9753882/attachment.html>
More information about the llvm-commits
mailing list