[InstCombine] Check nowrap flags when folding comparison of GEPs with the same base pointer #121892
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
llvm:instcombine
Member
|
@llvm/pr-subscribers-llvm-transforms Author: Yingwei Zheng (dtcxzyw) ChangesAlive2: https://alive2.llvm.org/ce/z/P5XbMx TODO: It is still safe to perform this transform without nowrap flags if the corresponding scale factor is 1 byte: https://alive2.llvm.org/ce/z/J-JCJd Full diff: https://github.com/llvm/llvm-project/pull/121892.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 8b23583c510637..9f136860724db8 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -833,8 +833,11 @@ Instruction *InstCombinerImpl::foldGEPICmp(GEPOperator *GEPLHS, Value *RHS,
if (NumDifferences == 0) // SAME GEP?
return replaceInstUsesWith(I, // No comparison is needed here.
ConstantInt::get(I.getType(), ICmpInst::isTrueWhenEqual(Cond)));
-
- else if (NumDifferences == 1 && CanFold(NW)) {
+ // If two GEPs only differ by an index/the base pointer, compare them.
+ // Note that nowrap flags are always needed when comparing two indices.
+ else if (NumDifferences == 1 &&
+ (DiffOperand == 0 ? CanFold(NW)
+ : NW != GEPNoWrapFlags::none())) {
Value *LHSV = GEPLHS->getOperand(DiffOperand);
Value *RHSV = GEPRHS->getOperand(DiffOperand);
return NewICmp(NW, LHSV, RHSV);
diff --git a/llvm/test/Transforms/InstCombine/opaque-ptr.ll b/llvm/test/Transforms/InstCombine/opaque-ptr.ll
index b05274658e812e..be734243d14a10 100644
--- a/llvm/test/Transforms/InstCombine/opaque-ptr.ll
+++ b/llvm/test/Transforms/InstCombine/opaque-ptr.ll
@@ -467,6 +467,41 @@ define i1 @cmp_gep_same_base_same_type(ptr %ptr, i64 %idx1, i64 %idx2) {
ret i1 %cmp
}
+define i1 @cmp_gep_same_base_same_type_maywrap(ptr %ptr, i64 %idx1, i64 %idx2) {
+; CHECK-LABEL: @cmp_gep_same_base_same_type_maywrap(
+; CHECK-NEXT: [[CMP_UNSHIFTED:%.*]] = xor i64 [[IDX1:%.*]], [[IDX2:%.*]]
+; CHECK-NEXT: [[CMP_MASK:%.*]] = and i64 [[CMP_UNSHIFTED]], 4611686018427387903
+; CHECK-NEXT: [[CMP:%.*]] = icmp eq i64 [[CMP_MASK]], 0
+; CHECK-NEXT: ret i1 [[CMP]]
+;
+ %gep1 = getelementptr i32, ptr %ptr, i64 %idx1
+ %gep2 = getelementptr i32, ptr %ptr, i64 %idx2
+ %cmp = icmp eq ptr %gep1, %gep2
+ ret i1 %cmp
+}
+
+define i1 @cmp_gep_same_base_same_type_nuw(ptr %ptr, i64 %idx1, i64 %idx2) {
+; CHECK-LABEL: @cmp_gep_same_base_same_type_nuw(
+; CHECK-NEXT: [[CMP:%.*]] = icmp eq i64 [[IDX1:%.*]], [[IDX2:%.*]]
+; CHECK-NEXT: ret i1 [[CMP]]
+;
+ %gep1 = getelementptr nuw i32, ptr %ptr, i64 %idx1
+ %gep2 = getelementptr nuw i32, ptr %ptr, i64 %idx2
+ %cmp = icmp eq ptr %gep1, %gep2
+ ret i1 %cmp
+}
+
+define i1 @cmp_gep_same_base_same_type_nusw(ptr %ptr, i64 %idx1, i64 %idx2) {
+; CHECK-LABEL: @cmp_gep_same_base_same_type_nusw(
+; CHECK-NEXT: [[CMP:%.*]] = icmp eq i64 [[IDX1:%.*]], [[IDX2:%.*]]
+; CHECK-NEXT: ret i1 [[CMP]]
+;
+ %gep1 = getelementptr nusw i32, ptr %ptr, i64 %idx1
+ %gep2 = getelementptr nusw i32, ptr %ptr, i64 %idx2
+ %cmp = icmp eq ptr %gep1, %gep2
+ ret i1 %cmp
+}
+
define i1 @cmp_gep_same_base_different_type(ptr %ptr, i64 %idx1, i64 %idx2) {
; CHECK-LABEL: @cmp_gep_same_base_different_type(
; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i64 [[IDX1:%.*]], 2
|
This was referenced Jan 7, 2025
nikic
reviewed
Jan 7, 2025
| // If two GEPs only differ by an index/the base pointer, compare them. | ||
| // Note that nowrap flags are always needed when comparing two indices. | ||
| else if (NumDifferences == 1 && | ||
| (DiffOperand == 0 ? CanFold(NW) |
Contributor
There was a problem hiding this comment.
I don't think DiffOperand == 0 can happen here? The above loop only goes over the index operands. The case of base pointer difference is handled by different code further up.
nikic
approved these changes
Jan 7, 2025
Contributor
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
LGTM. We'll still fall back to the generic handling below, so I think the simple check is fine.
Contributor
|
Reverse ping :) |
Member
Author
Is it necessary to merge this PR now? I was previously planning to merge it after fixing the regression. |
Contributor
As it fixes a miscompile and the regression doesn't seem particularly significant, I think it's better to merge it earlier than later. |
dtcxzyw
force-pushed
the
perf/fix-121890
branch
from
b06fad7 to
5b0038c
Compare
dtcxzyw
force-pushed
the
perf/fix-121890
branch
from
5b0038c to
1cf4654
Compare
Contributor
|
/cherry-pick 9725595 |
Member
|
Failed to cherry-pick: 9725595 https://github.com/llvm/llvm-project/actions/runs/13097312131 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
nikic
pushed a commit
to nikic/llvm-project
that referenced
this pull request
Feb 5, 2025
… the same base pointer (llvm#121892) Alive2: https://alive2.llvm.org/ce/z/P5XbMx Closes llvm#121890 TODO: It is still safe to perform this transform without nowrap flags if the corresponding scale factor is 1 byte: https://alive2.llvm.org/ce/z/J-JCJd (cherry picked from commit 9725595)
nikic
pushed a commit
to nikic/llvm-project
that referenced
this pull request
Feb 11, 2025
… the same base pointer (llvm#121892) Alive2: https://alive2.llvm.org/ce/z/P5XbMx Closes llvm#121890 TODO: It is still safe to perform this transform without nowrap flags if the corresponding scale factor is 1 byte: https://alive2.llvm.org/ce/z/J-JCJd (cherry picked from commit 9725595)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alive2: https://alive2.llvm.org/ce/z/P5XbMx
Closes #121890
TODO: It is still safe to perform this transform without nowrap flags if the corresponding scale factor is 1 byte: https://alive2.llvm.org/ce/z/J-JCJd