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