[Utils] Fix incorrect LCSSA PHI nodes when splitting critical edges with MergeIdenticalEdges #131744
Conversation
…ith 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.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-transforms Author: None (Camsyn) ChangesDescriptionThis PR fixes incorrect LCSSA PHI node generation when splitting critical edges with both Issue DetailsWhen splitting edges from blocks with >1 identical edges to a loop exit block (e.g., via
Example (simplified from function ; Original IR
exiting:
switch i8 %cond, label %while.body [
i8 0, label %exit
i8 10, label %exit
]
exit:
%phi = phi i32 [%val, %exiting], [%val, %exiting] ; Requires two predecessors After splitting with split:
%lcssa = phi i32 [%val, %exiting] ; Missing one predecessor!
br label %exit instead of the correct: split:
%lcssa = phi i32 [%val, %exiting], [%val, %exiting] ; Both edges preserved
br label %exit FixModify ImpactFixes miscompilations in edge cases where critical edge splitting interacts with LCSSA and References
Full diff: https://github.com/llvm/llvm-project/pull/131744.diff 2 Files Affected:
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(
|
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
It looks like the test is failing: |
Co-authored-by: Nikita Popov <github@npopov.com>
|
Can this PR be merged? Or if there are any deficiencies, I will actively promote it. |
|
@Camsyn Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Description
This PR fixes incorrect LCSSA PHI node generation when splitting critical edges with both
PreserveLCSSAandMergeIdenticalEdgesenabled. The bug caused PHI nodes in the split blockto miss predecessors when multiple identical edges were merged.
Issue Details
When splitting edges from blocks with >1 identical edges to a loop exit block (e.g., via
switchinstructions), the interaction between these options leads to invalid PHI nodes:
number of identical edges.
Example (simplified from function
Parseinlemon.cfrom SQLite3 in FuzzBench):After splitting with
PreserveLCSSAandMergeIdenticalEdges, the PHI node in the split blockincorrectly became:
instead of the correct:
Fix
Modify
createPHIsForSplitLoopExitto accept all predecessor entries corresponding to the numberof merged edges. Instead of passing a single predecessor block, we now pass
NumSplittedIdenticalEdgescopies of the original block, ensuring PHI nodes have correct incoming entries.
Impact
Fixes miscompilations in edge cases where critical edge splitting interacts with LCSSA and
edge merging. No known regressions introduced.
References
BreakCriticalEdges.cpp(lines 291)Parsefunction inlemon.c.