[InstCombine] Remove one-use restriction on icmp of gep fold #76730
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
The fold for icmp (gep (p, i1), gep (p, i2)) to icmp (i1, i2) is currently limited to one of the GEPs either having one use or a constant offset. I believe this is to avoid duplicating complex arithmetic both in the GEP and the offset comparison. This patch instead does the same thing that the indexed compare fold does, which is to rewrite the GEP into i8 form if necessary, so that the offset arithmetic is not repeated after the transform.
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesThe fold for icmp (gep (p, i1), gep (p, i2)) to icmp (i1, i2) is currently limited to one of the GEPs either having one use or a constant offset. I believe this is to avoid duplicating complex arithmetic both in the GEP and the offset comparison. This patch instead does the same thing that the indexed compare fold does, which is to rewrite the GEP into i8 form if necessary, so that the offset arithmetic is not repeated after the transform. I ran into this problem in a case where there are multiple conditions on the same pointer, which prevents them from getting folded. Full diff: https://github.com/llvm/llvm-project/pull/76730.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 3875e59c3ede3b..ce928e40cdade0 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -813,14 +813,28 @@ Instruction *InstCombinerImpl::foldGEPICmp(GEPOperator *GEPLHS, Value *RHS,
}
}
- // Only lower this if the icmp is the only user of the GEP or if we expect
- // the result to fold to a constant!
- if ((GEPsInBounds || CmpInst::isEquality(Cond)) &&
- (GEPLHS->hasAllConstantIndices() || GEPLHS->hasOneUse()) &&
- (GEPRHS->hasAllConstantIndices() || GEPRHS->hasOneUse())) {
+ if (GEPsInBounds || CmpInst::isEquality(Cond)) {
+ auto EmitGEPOffsetAndRewrite = [&](GEPOperator *GEP) {
+ auto *Inst = dyn_cast<Instruction>(GEP);
+ if (Inst)
+ Builder.SetInsertPoint(Inst);
+
+ Value *Offset = EmitGEPOffset(GEP);
+ // If a non-trivial GEP has other uses, rewrite it to avoid duplicating
+ // the offset arithmetic.
+ if (Inst && !GEP->hasOneUse() && !GEP->hasAllConstantIndices()) {
+ replaceInstUsesWith(*Inst,
+ Builder.CreateGEP(Builder.getInt8Ty(),
+ GEP->getPointerOperand(),
+ Offset, "", GEPsInBounds));
+ eraseInstFromFunction(*Inst);
+ }
+ return Offset;
+ };
+
// ((gep Ptr, OFFSET1) cmp (gep Ptr, OFFSET2) ---> (OFFSET1 cmp OFFSET2)
- Value *L = EmitGEPOffset(GEPLHS);
- Value *R = EmitGEPOffset(GEPRHS);
+ Value *L = EmitGEPOffsetAndRewrite(GEPLHS);
+ Value *R = EmitGEPOffsetAndRewrite(GEPRHS);
return new ICmpInst(ICmpInst::getSignedPredicate(Cond), L, R);
}
}
diff --git a/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll b/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll
index 330825e1055eaa..f0e8962b1b6a64 100644
--- a/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll
@@ -40,8 +40,8 @@ define i1 @test59_as1(ptr addrspace(1) %foo) {
define i1 @test60(ptr %foo, i64 %i, i64 %j) {
; CHECK-LABEL: @test60(
; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[I:%.*]] to i32
-; CHECK-NEXT: [[TMP2:%.*]] = trunc i64 [[J:%.*]] to i32
; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i32 [[TMP1]], 2
+; CHECK-NEXT: [[TMP2:%.*]] = trunc i64 [[J:%.*]] to i32
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[GEP1_IDX]], [[TMP2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
@@ -54,8 +54,8 @@ define i1 @test60(ptr %foo, i64 %i, i64 %j) {
define i1 @test60_as1(ptr addrspace(1) %foo, i64 %i, i64 %j) {
; CHECK-LABEL: @test60_as1(
; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[I:%.*]] to i16
-; CHECK-NEXT: [[TMP2:%.*]] = trunc i64 [[J:%.*]] to i16
; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i16 [[TMP1]], 2
+; CHECK-NEXT: [[TMP2:%.*]] = trunc i64 [[J:%.*]] to i16
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i16 [[GEP1_IDX]], [[TMP2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
diff --git a/llvm/test/Transforms/InstCombine/icmp-gep.ll b/llvm/test/Transforms/InstCombine/icmp-gep.ll
index fb297e04ceb065..42e15e78975e51 100644
--- a/llvm/test/Transforms/InstCombine/icmp-gep.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-gep.ll
@@ -313,8 +313,8 @@ define i1 @test_gep_eq_no_inbounds(ptr %foo, i64 %i, i64 %j) {
define i1 @test60_as1(ptr addrspace(1) %foo, i64 %i, i64 %j) {
; CHECK-LABEL: @test60_as1(
; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[I:%.*]] to i16
-; CHECK-NEXT: [[TMP2:%.*]] = trunc i64 [[J:%.*]] to i16
; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i16 [[TMP1]], 2
+; CHECK-NEXT: [[TMP2:%.*]] = trunc i64 [[J:%.*]] to i16
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i16 [[GEP1_IDX]], [[TMP2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
@@ -400,11 +400,13 @@ define i1 @test61_as1(ptr addrspace(1) %foo, i16 %i, i16 %j) {
define i1 @test60_extra_use(ptr %foo, i64 %i, i64 %j) {
; CHECK-LABEL: @test60_extra_use(
-; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i32, ptr [[FOO:%.*]], i64 [[I:%.*]]
-; CHECK-NEXT: [[GEP2:%.*]] = getelementptr inbounds i16, ptr [[FOO]], i64 [[J:%.*]]
+; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i64 [[I:%.*]], 2
+; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i8, ptr [[FOO:%.*]], i64 [[GEP1_IDX]]
+; CHECK-NEXT: [[GEP2_IDX:%.*]] = shl nsw i64 [[J:%.*]], 1
+; CHECK-NEXT: [[GEP2:%.*]] = getelementptr inbounds i8, ptr [[FOO]], i64 [[GEP2_IDX]]
; CHECK-NEXT: call void @use(ptr [[GEP1]])
; CHECK-NEXT: call void @use(ptr [[GEP2]])
-; CHECK-NEXT: [[CMP:%.*]] = icmp ult ptr [[GEP1]], [[GEP2]]
+; CHECK-NEXT: [[CMP:%.*]] = icmp slt i64 [[GEP1_IDX]], [[GEP2_IDX]]
; CHECK-NEXT: ret i1 [[CMP]]
;
%gep1 = getelementptr inbounds i32, ptr %foo, i64 %i
@@ -446,13 +448,14 @@ define i1 @test60_extra_use_const_operands_no_inbounds(ptr %foo, i64 %i, i64 %j)
define void @test60_extra_use_fold(ptr %foo, i64 %start.idx, i64 %end.offset) {
; CHECK-LABEL: @test60_extra_use_fold(
-; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i32, ptr [[FOO:%.*]], i64 [[START_IDX:%.*]]
+; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nsw i64 [[START_IDX:%.*]], 2
+; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i8, ptr [[FOO:%.*]], i64 [[GEP1_IDX]]
; CHECK-NEXT: [[GEP2:%.*]] = getelementptr inbounds i8, ptr [[FOO]], i64 [[END_OFFSET:%.*]]
; CHECK-NEXT: call void @use(ptr [[GEP1]])
; CHECK-NEXT: call void @use(ptr [[GEP2]])
-; CHECK-NEXT: [[CMP1:%.*]] = icmp eq ptr [[GEP1]], [[GEP2]]
+; CHECK-NEXT: [[CMP1:%.*]] = icmp eq i64 [[GEP1_IDX]], [[END_OFFSET]]
; CHECK-NEXT: call void @use.i1(i1 [[CMP1]])
-; CHECK-NEXT: [[CMP2:%.*]] = icmp ult ptr [[GEP1]], [[GEP2]]
+; CHECK-NEXT: [[CMP2:%.*]] = icmp slt i64 [[GEP1_IDX]], [[END_OFFSET]]
; CHECK-NEXT: call void @use.i1(i1 [[CMP2]])
; CHECK-NEXT: ret void
;
|
dtcxzyw
added a commit
to dtcxzyw/llvm-opt-benchmark
that referenced
this pull request
Jan 2, 2024
dtcxzyw
reviewed
Jan 2, 2024
dtcxzyw
reviewed
Jan 3, 2024
|
Ping. |
|
Huh, this change had a suspiciously large impact on |
|
Looks like The final version of the function is getting additional simplification relative to the baseline -- one of the pointer comparisons now gets optimized away. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The fold for icmp (gep (p, i1), gep (p, i2)) to icmp (i1, i2) is currently limited to one of the GEPs either having one use or a constant offset. I believe this is to avoid duplicating complex arithmetic both in the GEP and the offset comparison.
This patch instead does the same thing that the indexed compare fold does, which is to rewrite the GEP into i8 form if necessary, so that the offset arithmetic is not repeated after the transform.
I ran into this problem in a case where there are multiple conditions on the same pointer, which prevents them from getting folded.