[llvm] 6aa3fc4 - Revert "[InstCombine] propagate 'nsw' on pointer difference of 'inbounds' geps (PR47430)"
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 4 14:51:16 PST 2020
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/20201104/fd258bca/attachment.html>
More information about the llvm-commits
mailing list