[InstCombine] Fix pointer replacement in foldSelectValueEquivalence
#161701
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
|
@llvm/pr-subscribers-llvm-transforms Author: Yingwei Zheng (dtcxzyw) ChangesCloses #161636. Compile-time impact (+0.02%): https://llvm-compile-time-tracker.com/compare.php?from=17f6888d1771c9f61378a0a58725f3359277ddda&to=44cdcb84b1a9d996cd61f2c37ac4c9df5bc531af&stat=instructions%3Au
Perhaps we can cache the query result of Full diff: https://github.com/llvm/llvm-project/pull/161701.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 87000a1c36eef..5ea79b92633d9 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -17,6 +17,7 @@
#include "llvm/Analysis/AssumptionCache.h"
#include "llvm/Analysis/CmpInstAnalysis.h"
#include "llvm/Analysis/InstructionSimplify.h"
+#include "llvm/Analysis/Loads.h"
#include "llvm/Analysis/OverflowInstAnalysis.h"
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/Analysis/VectorUtils.h"
@@ -1411,6 +1412,8 @@ Instruction *InstCombinerImpl::foldSelectValueEquivalence(SelectInst &Sel,
// in the cmp and in f(Y).
if (TrueVal == OldOp && (isa<Constant>(OldOp) || !isa<Constant>(NewOp)))
return nullptr;
+ if (!canReplacePointersIfEqual(OldOp, NewOp, DL))
+ return nullptr;
if (Value *V = simplifyWithOpReplaced(TrueVal, OldOp, NewOp, SQ,
/* AllowRefinement=*/true)) {
@@ -1466,12 +1469,14 @@ Instruction *InstCombinerImpl::foldSelectValueEquivalence(SelectInst &Sel,
// Example:
// (X == 42) ? 43 : (X + 1) --> (X == 42) ? (X + 1) : (X + 1) --> X + 1
SmallVector<Instruction *> DropFlags;
- if (simplifyWithOpReplaced(FalseVal, CmpLHS, CmpRHS, SQ,
- /* AllowRefinement */ false,
- &DropFlags) == TrueVal ||
- simplifyWithOpReplaced(FalseVal, CmpRHS, CmpLHS, SQ,
- /* AllowRefinement */ false,
- &DropFlags) == TrueVal) {
+ if ((canReplacePointersIfEqual(CmpLHS, CmpRHS, DL) &&
+ simplifyWithOpReplaced(FalseVal, CmpLHS, CmpRHS, SQ,
+ /* AllowRefinement */ false,
+ &DropFlags) == TrueVal) ||
+ (canReplacePointersIfEqual(CmpRHS, CmpLHS, DL) &&
+ simplifyWithOpReplaced(FalseVal, CmpRHS, CmpLHS, SQ,
+ /* AllowRefinement */ false,
+ &DropFlags) == TrueVal)) {
for (Instruction *I : DropFlags) {
I->dropPoisonGeneratingAnnotations();
Worklist.add(I);
diff --git a/llvm/test/Transforms/InstCombine/select-gep.ll b/llvm/test/Transforms/InstCombine/select-gep.ll
index dd8dffba11b05..718133699a8a7 100644
--- a/llvm/test/Transforms/InstCombine/select-gep.ll
+++ b/llvm/test/Transforms/InstCombine/select-gep.ll
@@ -286,3 +286,35 @@ define <2 x ptr> @test7(<2 x ptr> %p1, i64 %idx, <2 x i1> %cc) {
%select = select <2 x i1> %cc, <2 x ptr> %p1, <2 x ptr> %gep
ret <2 x ptr> %select
}
+
+define ptr @ptr_eq_replace_freeze1(ptr %p, ptr %q) {
+; CHECK-LABEL: @ptr_eq_replace_freeze1(
+; CHECK-NEXT: [[Q_FR:%.*]] = freeze ptr [[Q:%.*]]
+; CHECK-NEXT: [[Q_FR1:%.*]] = freeze ptr [[Q1:%.*]]
+; CHECK-NEXT: [[CMP:%.*]] = icmp eq ptr [[Q_FR]], [[Q_FR1]]
+; CHECK-NEXT: [[SELECT:%.*]] = select i1 [[CMP]], ptr [[Q_FR]], ptr [[Q_FR1]]
+; CHECK-NEXT: ret ptr [[SELECT]]
+;
+ %p.fr = freeze ptr %p
+ %q.fr = freeze ptr %q
+ %cmp = icmp eq ptr %p.fr, %q.fr
+ %select = select i1 %cmp, ptr %p.fr, ptr %q.fr
+ ret ptr %select
+}
+
+define ptr @ptr_eq_replace_freeze2(ptr %p, ptr %q) {
+; CHECK-LABEL: @ptr_eq_replace_freeze2(
+; CHECK-NEXT: [[P_FR:%.*]] = freeze ptr [[P:%.*]]
+; CHECK-NEXT: [[P_FR1:%.*]] = freeze ptr [[P1:%.*]]
+; CHECK-NEXT: [[CMP:%.*]] = icmp eq ptr [[P_FR1]], [[P_FR]]
+; CHECK-NEXT: [[SELECT_V:%.*]] = select i1 [[CMP]], ptr [[P_FR1]], ptr [[P_FR]]
+; CHECK-NEXT: [[SELECT:%.*]] = getelementptr i8, ptr [[SELECT_V]], i64 16
+; CHECK-NEXT: ret ptr [[SELECT]]
+;
+ %gep1 = getelementptr i32, ptr %p, i64 4
+ %gep2 = getelementptr i32, ptr %q, i64 4
+ %cmp = icmp eq ptr %p, %q
+ %cmp.fr = freeze i1 %cmp
+ %select = select i1 %cmp.fr, ptr %gep1, ptr %gep2
+ ret ptr %select
+}
|
|
Maybe I'm missing something, but at least from the tests, aren't the three canReplacePointersIfEqual being invoked on the same pointers all the times? Couldn't this be done only once? |
It is not commutative. You can always replace a pointer with a known dereferencable one. |
Sorry, I'm not clear. I do understand isDereferenceablePointer is called on different pointers, though, now, canReplacePointersIfEqual(CmpLHS, CmpRHS) is being called twice (one in ReplaceOldOpWithNewOp, one later), and so is canReplacePointersIfEqual(CmpRHS, CmpLHS). Why cannot this be done once for both the operands? |
I will cache the result. |
Thanks. BTW, what I had in mind originally was something along the following lines: bool RP1 = canReplacePointersIfEqual(CmpLHS, CmpRHS, DL);
if (Instruction *R = ReplaceOldOpWithNewOp(CmpLHS, CmpRHS, RP1))
return R;
bool RP2 = canReplacePointersIfEqual(CmpRHS, CmpLHS, DL);
if (Instruction *R = ReplaceOldOpWithNewOp(CmpRHS, CmpLHS, RP2))
return R;And later |
nikic
reviewed
Oct 2, 2025
| ; CHECK-NEXT: ret ptr [[SELECT]] | ||
| ; | ||
| %p.fr = freeze ptr %p | ||
| %q.fr = freeze ptr %q |
There was a problem hiding this comment.
What is the significance of freeze here? Why does this issue not reproduce without it (or with just noundef)?
There was a problem hiding this comment.
It is guarded by
llvm-project/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
Lines 1456 to 1458 in bd7e228
|
I don't think we really need the caching. There doesn't seem to be any significant compile-time impact, so I'd rather not duplicate this logic. |
nikic
reviewed
Oct 4, 2025
| auto ReplaceOldOpWithNewOp = [&](Value *OldOp, | ||
| Value *NewOp) -> Instruction * { | ||
| auto ReplaceOldOpWithNewOp = [&](Value *OldOp, Value *NewOp, | ||
| uint32_t Direction) -> Instruction * { |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/24687 Here is the relevant piece of the build log for the reference |
aokblast
pushed a commit
to aokblast/llvm-project
that referenced
this pull request
Oct 6, 2025
…llvm#161701) Closes llvm#161636. Compile-time impact (+0.06%): https://llvm-compile-time-tracker.com/compare.php?from=c2ef022aa7413ddc9aba48fa6fbe6fbd0cb14e19&to=9a0f0302efc30580136d191e66bac929f08ee25f&stat=instructions%3Au I used to disable this fold for pointers, because I cannot construct a positive test that is covered by `foldSelectValueEquivalence ` but not covered by `simplifySelectWithICmpCond`. But the IR diff shows we still benefit from the fold in InstCombine: + Bail out on pointers: dtcxzyw/llvm-opt-benchmark#2880 + This patch: dtcxzyw/llvm-opt-benchmark#2882
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.
Closes #161636.
Compile-time impact (+0.06%): https://llvm-compile-time-tracker.com/compare.php?from=c2ef022aa7413ddc9aba48fa6fbe6fbd0cb14e19&to=9a0f0302efc30580136d191e66bac929f08ee25f&stat=instructions%3Au
I used to disable this fold for pointers, because I cannot construct a positive test that is covered by
foldSelectValueEquivalencebut not covered bysimplifySelectWithICmpCond. But the IR diff shows we still benefit from the fold in InstCombine: