[CodeGenPrepare] Don't simplify incomplete expression tree in AddrModeCombine #164628
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
|
@llvm/pr-subscribers-llvm-transforms Author: Yingwei Zheng (dtcxzyw) ChangesSince new select/phi instructions may construct loops, the expression tree to be simplified may still be incomplete (i.e., it may contain select with dummy values or phi without incoming values). This patch removes the call to simplifyInstruction for now, as it doesn't break existing tests. Original PR: https://reviews.llvm.org/D36073 Full diff: https://github.com/llvm/llvm-project/pull/164628.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 9e78ec96a2f27..8ea132626a5af 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -4030,7 +4030,6 @@ bool PhiNodeSetIterator::operator!=(const PhiNodeSetIterator &RHS) const {
/// if it is simplified.
class SimplificationTracker {
DenseMap<Value *, Value *> Storage;
- const SimplifyQuery &SQ;
// Tracks newly created Phi nodes. The elements are iterated by insertion
// order.
PhiNodeSet AllPhiNodes;
@@ -4038,8 +4037,6 @@ class SimplificationTracker {
SmallPtrSet<SelectInst *, 32> AllSelectNodes;
public:
- SimplificationTracker(const SimplifyQuery &sq) : SQ(sq) {}
-
Value *Get(Value *V) {
do {
auto SV = Storage.find(V);
@@ -4049,30 +4046,6 @@ class SimplificationTracker {
} while (true);
}
- Value *Simplify(Value *Val) {
- SmallVector<Value *, 32> WorkList;
- SmallPtrSet<Value *, 32> Visited;
- WorkList.push_back(Val);
- while (!WorkList.empty()) {
- auto *P = WorkList.pop_back_val();
- if (!Visited.insert(P).second)
- continue;
- if (auto *PI = dyn_cast<Instruction>(P))
- if (Value *V = simplifyInstruction(cast<Instruction>(PI), SQ)) {
- for (auto *U : PI->users())
- WorkList.push_back(cast<Value>(U));
- Put(PI, V);
- PI->replaceAllUsesWith(V);
- if (auto *PHI = dyn_cast<PHINode>(PI))
- AllPhiNodes.erase(PHI);
- if (auto *Select = dyn_cast<SelectInst>(PI))
- AllSelectNodes.erase(Select);
- PI->eraseFromParent();
- }
- }
- return Get(Val);
- }
-
void Put(Value *From, Value *To) { Storage.insert({From, To}); }
void ReplacePhi(PHINode *From, PHINode *To) {
@@ -4133,8 +4106,7 @@ class AddressingModeCombiner {
/// Common Type for all different fields in addressing modes.
Type *CommonType = nullptr;
- /// SimplifyQuery for simplifyInstruction utility.
- const SimplifyQuery &SQ;
+ const DataLayout &DL;
/// Original Address.
Value *Original;
@@ -4143,8 +4115,8 @@ class AddressingModeCombiner {
Value *CommonValue = nullptr;
public:
- AddressingModeCombiner(const SimplifyQuery &_SQ, Value *OriginalValue)
- : SQ(_SQ), Original(OriginalValue) {}
+ AddressingModeCombiner(const DataLayout &DL, Value *OriginalValue)
+ : DL(DL), Original(OriginalValue) {}
~AddressingModeCombiner() { eraseCommonValueIfDead(); }
@@ -4256,7 +4228,7 @@ class AddressingModeCombiner {
// Keep track of keys where the value is null. We will need to replace it
// with constant null when we know the common type.
SmallVector<Value *, 2> NullValue;
- Type *IntPtrTy = SQ.DL.getIntPtrType(AddrModes[0].OriginalValue->getType());
+ Type *IntPtrTy = DL.getIntPtrType(AddrModes[0].OriginalValue->getType());
for (auto &AM : AddrModes) {
Value *DV = AM.GetFieldAsValue(DifferentField, IntPtrTy);
if (DV) {
@@ -4306,7 +4278,7 @@ class AddressingModeCombiner {
// simplification is possible only if original phi/selects were not
// simplified yet.
// Using this mapping we can find the current value in AddrToBase.
- SimplificationTracker ST(SQ);
+ SimplificationTracker ST;
// First step, DFS to create PHI nodes for all intermediate blocks.
// Also fill traverse order for the second step.
@@ -4465,7 +4437,6 @@ class AddressingModeCombiner {
PHI->addIncoming(ST.Get(Map[PV]), B);
}
}
- Map[Current] = ST.Simplify(V);
}
}
@@ -5856,8 +5827,7 @@ bool CodeGenPrepare::optimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
// the graph are compatible.
bool PhiOrSelectSeen = false;
SmallVector<Instruction *, 16> AddrModeInsts;
- const SimplifyQuery SQ(*DL, TLInfo);
- AddressingModeCombiner AddrModes(SQ, Addr);
+ AddressingModeCombiner AddrModes(*DL, Addr);
TypePromotionTransaction TPT(RemovedInsts);
TypePromotionTransaction::ConstRestorationPt LastKnownGood =
TPT.getRestorationPoint();
diff --git a/llvm/test/Transforms/CodeGenPrepare/X86/sink-addrmode-base.ll b/llvm/test/Transforms/CodeGenPrepare/X86/sink-addrmode-base.ll
index 63fd1845594c6..2570b3b2efc31 100644
--- a/llvm/test/Transforms/CodeGenPrepare/X86/sink-addrmode-base.ll
+++ b/llvm/test/Transforms/CodeGenPrepare/X86/sink-addrmode-base.ll
@@ -951,3 +951,41 @@ fallthrough:
%v = add i32 %v1, %v2
ret i32 %v
}
+
+; Make sure we don't simplify an incomplete expression tree.
+define i8 @pr163453(ptr %p, i1 %cond) {
+; CHECK-LABEL: define i8 @pr163453(
+; CHECK-SAME: ptr [[P:%.*]], i1 [[COND:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[P_ADDR_0:%.*]] = getelementptr i8, ptr [[P]], i64 1
+; CHECK-NEXT: [[TMP0:%.*]] = load i8, ptr [[P]], align 1
+; CHECK-NEXT: [[INCDEC_PTR11:%.*]] = getelementptr i8, ptr [[P]], i64 2
+; CHECK-NEXT: [[SPEC_SELECT:%.*]] = select i1 [[COND]], ptr [[P_ADDR_0]], ptr [[INCDEC_PTR11]]
+; CHECK-NEXT: [[LOAD:%.*]] = load i8, ptr [[SPEC_SELECT]], align 1
+; CHECK-NEXT: ret i8 [[LOAD]]
+;
+entry:
+ br label %for.cond
+
+for.cond:
+ %p.pn = phi ptr [ %p, %entry ], [ %p.addr.0, %for.inc ]
+ %p.addr.0 = getelementptr i8, ptr %p.pn, i64 1
+ br i1 false, label %exit, label %for.body
+
+for.body:
+ %1 = load i8, ptr %p.pn, align 1
+ br i1 false, label %for.inc, label %if.else
+
+if.else:
+ %incdec.ptr11 = getelementptr i8, ptr %p.pn, i64 2
+ %spec.select = select i1 %cond, ptr %p.addr.0, ptr %incdec.ptr11
+ br label %exit
+
+for.inc:
+ br label %for.cond
+
+exit:
+ %p.addr.3 = phi ptr [ %spec.select, %if.else ], [ %p.addr.0, %for.cond ]
+ %load = load i8, ptr %p.addr.3, align 1
+ ret i8 %load
+}
|
nikic
reviewed
Oct 22, 2025
There was a problem hiding this comment.
Does SimplificationTracker still do something useful if the Simplify() part is dropped?
Original PR: https://reviews.llvm.org/D36073
There is some talk about tests in the review discussion, but the actual 400 loc commit does not have even a single test :(
|
Okay, looks like the tests landed in a separate commit for some reason: cde03f3 |
Yeah. It still maintains the set of newly inserted selects/PHI nodes. It also tracks the phi replacement if AddrCombiner finds a match. TBH I believe it needs some refactoring. But I don't like to touch this code :( |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/128/builds/8022 Here is the relevant piece of the build log for the reference |
CongzheUalberta
added a commit
that referenced
this pull request
Oct 26, 2025
varun-r-mallya
pushed a commit
to varun-r-mallya/llvm-project
that referenced
this pull request
Oct 27, 2025
…)" (llvm#164694) This reverts commit f1c1063. PR llvm#163453 was merged and reverted since it exposed a crash. After investigation the crash was unrelated and is then fixed in llvm#164628. This is an attempt to reland llvm#163453.
dvbuka
pushed a commit
to dvbuka/llvm-project
that referenced
this pull request
Oct 27, 2025
…eCombine (llvm#164628) Since new select/phi instructions may construct loops, the expression tree to be simplified may still be incomplete (i.e., it may contain select with dummy values or phi without incoming values). This patch removes the call to simplifyInstruction for now, as it doesn't break existing tests. Original PR: https://reviews.llvm.org/D36073 Fix the crash reported in llvm#163453 (comment).
dvbuka
pushed a commit
to dvbuka/llvm-project
that referenced
this pull request
Oct 27, 2025
…)" (llvm#164694) This reverts commit f1c1063. PR llvm#163453 was merged and reverted since it exposed a crash. After investigation the crash was unrelated and is then fixed in llvm#164628. This is an attempt to reland llvm#163453.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Since new select/phi instructions may construct loops, the expression tree to be simplified may still be incomplete (i.e., it may contain select with dummy values or phi without incoming values). This patch removes the call to simplifyInstruction for now, as it doesn't break existing tests.
Original PR: https://reviews.llvm.org/D36073
Fix the crash reported in #163453 (comment).