[llvm] [Utils] Fix incorrect LCSSA PHI nodes when splitting critical edges with MergeIdenticalEdges (PR #131744)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 20 03:52:40 PDT 2025
https://github.com/Camsyn updated https://github.com/llvm/llvm-project/pull/131744
>From 1563edcf81023e6d370df48ae1cf44ecfb1761f0 Mon Sep 17 00:00:00 2001
From: Camsyn <65994555+Camsyn at users.noreply.github.com>
Date: Tue, 18 Mar 2025 15:07:01 +0800
Subject: [PATCH 1/4] [Utils] Fix incorrect LCSSA PHI nodes when splitting
critical edges with MergeIdenticalEdges
When splitting a critical edge from a block with multiple identical
edges to an exit block while both `PreserveLCSSA` and
`MergeIdenticalEdges` are enabled, the generated LCSSA PHI
nodes in the split block would miss incoming values from merged edges.
This occurs because:
1. `MergeIdenticalEdges` merges multiple identical edges into a single
edge to the new split block.
2. `PreserveLCSSA` (in `createPHIsForSplitLoopExit`) previously assumed
only one incoming edge from the original block, creating PHI nodes
with incomplete predecessors.
The fix modifies `createPHIsForSplitLoopExit` to account for merged
edges by passing all original predecessor entries (matching the number
of merged edges) when creating PHI nodes.
This ensures all incoming values are properly reflected in the split
block's PHI nodes.
Add unittest case in `BasicBlockUtilsTest.cpp` to verify the correction
of PHI node generation in this scenario.
---
.../Transforms/Utils/BreakCriticalEdges.cpp | 11 +++-
.../Transforms/Utils/BasicBlockUtilsTest.cpp | 57 +++++++++++++++++++
2 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
index 62b4b545f29bb..075a1afb4f6d9 100644
--- a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
+++ b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
@@ -205,6 +205,8 @@ llvm::SplitKnownCriticalEdge(Instruction *TI, unsigned SuccNum,
}
}
+ unsigned NumSplittedIdenticalEdges = 1;
+
// If there are any other edges from TIBB to DestBB, update those to go
// through the split block, making those edges non-critical as well (and
// reducing the number of phi entries in the DestBB if relevant).
@@ -217,6 +219,9 @@ llvm::SplitKnownCriticalEdge(Instruction *TI, unsigned SuccNum,
// We found another edge to DestBB, go to NewBB instead.
TI->setSuccessor(i, NewBB);
+
+ // Record the number of splitted identical edges to DestBB.
+ NumSplittedIdenticalEdges++;
}
}
@@ -288,7 +293,11 @@ llvm::SplitKnownCriticalEdge(Instruction *TI, unsigned SuccNum,
// Update LCSSA form in the newly created exit block.
if (Options.PreserveLCSSA) {
- createPHIsForSplitLoopExit(TIBB, NewBB, DestBB);
+ // If > 1 identical edges to be splitted, we need to introduce
+ // the incoming blocks of the same number for the new PHINode.
+ createPHIsForSplitLoopExit(
+ SmallVector<BasicBlock *, 4>(NumSplittedIdenticalEdges, TIBB),
+ NewBB, DestBB);
}
if (!LoopPreds.empty()) {
diff --git a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
index 56692cf25b797..a2f17ea672833 100644
--- a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
+++ b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
@@ -438,6 +438,63 @@ define void @crit_edge(i1 %cond0, i1 %cond1) {
EXPECT_TRUE(PDT.verify());
}
+TEST(BasicBlockUtils, SplitLoopCriticalEdge) {
+ LLVMContext C;
+ std::unique_ptr<Module> M = parseIR(C, R"IR(
+declare dso_local i1 @predicate(ptr noundef %p)
+
+define dso_local ptr @Parse(ptr noundef %gp) {
+entry:
+ br label %for.inc
+
+for.inc:
+ %phi = phi ptr [ %gp, %entry ], [ %cp, %while.cond ], [ %cp, %while.cond ]
+ %cond = call i1 @predicate(ptr noundef %phi)
+ %inc= getelementptr inbounds i8, ptr %phi, i64 1
+ br i1 %cond, label %while.cond, label %exit
+
+while.cond:
+ %cp = phi ptr [ %inc, %for.inc ], [ %incdec, %while.body ]
+ %val = load i8, ptr %cp, align 1
+ switch i8 %val, label %while.body [
+ i8 10, label %for.inc
+ i8 0, label %for.inc
+ ]
+
+while.body:
+ %incdec= getelementptr inbounds i8, ptr %cp, i64 1
+ br label %while.cond
+
+exit:
+ ret ptr %phi
+}
+)IR");
+ Function *F = M->getFunction("Parse");
+ DominatorTree DT(*F);
+ LoopInfo LI(DT);
+
+ CriticalEdgeSplittingOptions CESO =
+ CriticalEdgeSplittingOptions(nullptr, &LI, nullptr)
+ .setMergeIdenticalEdges()
+ .setPreserveLCSSA();
+ EXPECT_EQ(2u, SplitAllCriticalEdges(*F, CESO));
+
+ BasicBlock *WhileBB = getBasicBlockByName(*F, "while.cond");
+ BasicBlock *SplitBB = WhileBB->getTerminator()->getSuccessor(1);
+ // The only 1 successor of SplitBB is %for.inc
+ ASSERT_EQ(1u, SplitBB->getTerminator()->getNumSuccessors());
+ // MergeIdenticalEdges: SplitBB has two identical predecessors, %while.cond.
+ ASSERT_EQ(WhileBB, SplitBB->getUniquePredecessor());
+ ASSERT_EQ(true, SplitBB->hasNPredecessors(2));
+
+ PHINode *PN = dyn_cast<PHINode>(&(SplitBB->front()));
+ // PreserveLCSSA: should insert a PHI node in front of SplitBB
+ ASSERT_NE(nullptr, PN);
+ // The PHI node should have 2 identical incoming blocks.
+ ASSERT_EQ(2u, PN->getNumIncomingValues());
+ ASSERT_EQ(PN->getIncomingBlock(0), PN->getIncomingBlock(1));
+}
+
TEST(BasicBlockUtils, SplitIndirectBrCriticalEdgesIgnorePHIs) {
LLVMContext C;
std::unique_ptr<Module> M = parseIR(C, R"IR(
>From 7520d619452f50c679776125e9dbb7424efafc40 Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Thu, 20 Mar 2025 16:36:13 +0800
Subject: [PATCH 2/4] Updates adhering the commit suggestion
---
.../Transforms/Utils/BreakCriticalEdges.cpp | 14 ++++++------
.../Transforms/Utils/BasicBlockUtilsTest.cpp | 22 +++++++++----------
2 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
index 075a1afb4f6d9..50056b2efe009 100644
--- a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
+++ b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
@@ -205,7 +205,7 @@ llvm::SplitKnownCriticalEdge(Instruction *TI, unsigned SuccNum,
}
}
- unsigned NumSplittedIdenticalEdges = 1;
+ unsigned NumSplitIdenticalEdges = 1;
// If there are any other edges from TIBB to DestBB, update those to go
// through the split block, making those edges non-critical as well (and
@@ -220,8 +220,8 @@ llvm::SplitKnownCriticalEdge(Instruction *TI, unsigned SuccNum,
// We found another edge to DestBB, go to NewBB instead.
TI->setSuccessor(i, NewBB);
- // Record the number of splitted identical edges to DestBB.
- NumSplittedIdenticalEdges++;
+ // Record the number of split identical edges to DestBB.
+ NumSplitIdenticalEdges++;
}
}
@@ -293,11 +293,11 @@ llvm::SplitKnownCriticalEdge(Instruction *TI, unsigned SuccNum,
// Update LCSSA form in the newly created exit block.
if (Options.PreserveLCSSA) {
- // If > 1 identical edges to be splitted, we need to introduce
- // the incoming blocks of the same number for the new PHINode.
+ // If > 1 identical edges to be split, we need to introduce the same
+ // number of the incoming blocks for the new PHINode.
createPHIsForSplitLoopExit(
- SmallVector<BasicBlock *, 4>(NumSplittedIdenticalEdges, TIBB),
- NewBB, DestBB);
+ SmallVector<BasicBlock *, 4>(NumSplitIdenticalEdges, TIBB), NewBB,
+ DestBB);
}
if (!LoopPreds.empty()) {
diff --git a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
index a2f17ea672833..166388e20d78b 100644
--- a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
+++ b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
@@ -441,15 +441,15 @@ define void @crit_edge(i1 %cond0, i1 %cond1) {
TEST(BasicBlockUtils, SplitLoopCriticalEdge) {
LLVMContext C;
std::unique_ptr<Module> M = parseIR(C, R"IR(
-declare dso_local i1 @predicate(ptr noundef %p)
+declare i1 @predicate(ptr %p)
-define dso_local ptr @Parse(ptr noundef %gp) {
+define ptr @Parse(ptr %gp) {
entry:
br label %for.inc
for.inc:
%phi = phi ptr [ %gp, %entry ], [ %cp, %while.cond ], [ %cp, %while.cond ]
- %cond = call i1 @predicate(ptr noundef %phi)
+ %cond = call i1 @predicate(ptr %phi)
%inc= getelementptr inbounds i8, ptr %phi, i64 1
br i1 %cond, label %while.cond, label %exit
@@ -462,7 +462,7 @@ while.cond:
]
while.body:
- %incdec= getelementptr inbounds i8, ptr %cp, i64 1
+ %incdec = getelementptr inbounds i8, ptr %cp, i64 1
br label %while.cond
exit:
@@ -482,17 +482,17 @@ while.body:
BasicBlock *WhileBB = getBasicBlockByName(*F, "while.cond");
BasicBlock *SplitBB = WhileBB->getTerminator()->getSuccessor(1);
// The only 1 successor of SplitBB is %for.inc
- ASSERT_EQ(1u, SplitBB->getTerminator()->getNumSuccessors());
+ EXPECT_EQ(1u, SplitBB->getTerminator()->getNumSuccessors());
// MergeIdenticalEdges: SplitBB has two identical predecessors, %while.cond.
- ASSERT_EQ(WhileBB, SplitBB->getUniquePredecessor());
- ASSERT_EQ(true, SplitBB->hasNPredecessors(2));
+ EXPECT_EQ(WhileBB, SplitBB->getUniquePredecessor());
+ EXPECT_TRUE(SplitBB->hasNPredecessors(2));
- PHINode *PN = dyn_cast<PHINode>(&(SplitBB->front()));
+ auto *PN = dyn_cast<PHINode>(&SplitBB->front());
// PreserveLCSSA: should insert a PHI node in front of SplitBB
- ASSERT_NE(nullptr, PN);
+ EXPECT_EQ(nullptr, PN);
// The PHI node should have 2 identical incoming blocks.
- ASSERT_EQ(2u, PN->getNumIncomingValues());
- ASSERT_EQ(PN->getIncomingBlock(0), PN->getIncomingBlock(1));
+ EXPECT_EQ(2u, PN->getNumIncomingValues());
+ EXPECT_EQ(PN->getIncomingBlock(0), PN->getIncomingBlock(1));
}
TEST(BasicBlockUtils, SplitIndirectBrCriticalEdgesIgnorePHIs) {
>From 0cdc376e8457ce1a86c5d3a43ebc182cfc8f16af Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Thu, 20 Mar 2025 17:09:41 +0800
Subject: [PATCH 3/4] update to make formatter happy
---
llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
index 50056b2efe009..3667d03ff6943 100644
--- a/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
+++ b/llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
@@ -205,7 +205,7 @@ llvm::SplitKnownCriticalEdge(Instruction *TI, unsigned SuccNum,
}
}
- unsigned NumSplitIdenticalEdges = 1;
+ unsigned NumSplitIdenticalEdges = 1;
// If there are any other edges from TIBB to DestBB, update those to go
// through the split block, making those edges non-critical as well (and
>From e8333bfb8d77683f0f9515bcd88f62f2f0f9f4df Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Thu, 20 Mar 2025 18:52:31 +0800
Subject: [PATCH 4/4] Update
llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
Co-authored-by: Nikita Popov <github at npopov.com>
---
llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
index 166388e20d78b..c9a6a32851775 100644
--- a/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
+++ b/llvm/unittests/Transforms/Utils/BasicBlockUtilsTest.cpp
@@ -489,7 +489,7 @@ while.body:
auto *PN = dyn_cast<PHINode>(&SplitBB->front());
// PreserveLCSSA: should insert a PHI node in front of SplitBB
- EXPECT_EQ(nullptr, PN);
+ EXPECT_NE(nullptr, PN);
// The PHI node should have 2 identical incoming blocks.
EXPECT_EQ(2u, PN->getNumIncomingValues());
EXPECT_EQ(PN->getIncomingBlock(0), PN->getIncomingBlock(1));
More information about the llvm-commits
mailing list