[SimplifyCFG] Simplify identical predecessors - #173022
Merged
Merged
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
Camsyn
force-pushed
the
merge-dup-pred
branch
from
December 19, 2025 16:29
54d7a50 to
be1c964
Compare
Contributor
|
This probably subsumes simplifyDuplicateSwitchArms? |
Contributor
Author
Yes, it is indeed a generalization of |
Camsyn
force-pushed
the
merge-dup-pred
branch
from
December 20, 2025 07:45
be1c964 to
4e0b53a
Compare
Camsyn
commented
Dec 20, 2025
Camsyn
force-pushed
the
merge-dup-pred
branch
from
December 20, 2025 09:44
4e0b53a to
ade6af0
Compare
Camsyn
changed the title
Camsyn
force-pushed
the
merge-dup-pred
branch
from
December 20, 2025 10:13
ade6af0 to
185e648
Compare
Camsyn
commented
Dec 20, 2025
This was referenced Dec 20, 2025
Camsyn
force-pushed
the
merge-dup-pred
branch
2 times, most recently
from
December 22, 2025 09:13
b17f5b5 to
02da180
Compare
Camsyn
force-pushed
the
merge-dup-pred
branch
from
December 22, 2025 11:18
02da180 to
ca6c8da
Compare
Camsyn
commented
Dec 23, 2025
Comment on lines
+8978
to
+8980
| // Merge identical predecessors of this block | ||
| if (simplifyDuplicatePredecessors(BB, DTU)) | ||
| return true; |
Contributor
Author
There was a problem hiding this comment.
Should we treat identical predecessor merging as part of sink-common-insts, and perform it after sinkCommonCodeFromPredecessors?
This avoids interfering with earlier optimizations that rely on special basic block structures (e.g., unconditioned branch folding, speculativelyExecuteBB and jump-threading).
Contributor
There was a problem hiding this comment.
Yeah, we probably do not want to do this transform too early. Not sure whether having the same phase ordering as sink-common-insts is right, but it's probably a reasonable starting point...
This was referenced Dec 23, 2025
Camsyn
force-pushed
the
merge-dup-pred
branch
from
December 24, 2025 08:27
4647b4a to
95312c5
Compare
dtcxzyw
reviewed
Mar 4, 2026
| return false; | ||
|
|
||
| // The BB must have at least one predecessor. | ||
| if (!BB->hasNPredecessorsOrMore(1)) |
Member
There was a problem hiding this comment.
Suggested change
| if (!BB->hasNPredecessorsOrMore(1)) | |
| if (pred_empty(BB)) |
| return false; | ||
|
|
||
| // TODO: relax this condition to merge equal blocks with >1 instructions? | ||
| if (BB->size() != 1) |
Member
There was a problem hiding this comment.
It is calculated in linear time:
Contributor
Author
There was a problem hiding this comment.
This code is directly inherited from the original code.
Here, we can replace it with a O(1) judgment &BB->front() != &BB->back().
Contributor
Author
|
Seems the comptime issue has been resolved. |
nikic
reviewed
Mar 5, 2026
|
|
||
| // Avoid blocks that are "address-taken" (blockaddress) or have unusual | ||
| // uses. | ||
| if (BB->hasAddressTaken() || BB->isLandingPad()) |
Contributor
There was a problem hiding this comment.
It looks like these conditions don't have test coverage?
Also, are you sure you want to exclude landingpads specifically, rather than EH pads in general?
Contributor
Author
There was a problem hiding this comment.
Yes. We should use isEHPad().
I have added a new test to cover BB->hasAddressTaken() .
BB->isEHPad() is hard to cover as there are no TWO identical EHPad BBs with only ONE unconditioned branch.
I added this guard just for future usage.
| return false; | ||
|
|
||
| // TODO: relax this condition to merge equal blocks with >1 instructions? | ||
| if (/* I.e., O(n) calc: size() != 1 */ &BB->front() != &BB->back()) |
Contributor
There was a problem hiding this comment.
Move this comment out of the condition.
Comment on lines
+8063
to
+8064
| Succ->phis(), [BB, &PhiPredIVs = *EBW->PhiPredIVs](PHINode &Phi) { | ||
| return PhiPredIVs[&Phi][BB]; |
Contributor
There was a problem hiding this comment.
Suggested change
| Succ->phis(), [BB, &PhiPredIVs = *EBW->PhiPredIVs](PHINode &Phi) { | |
| return PhiPredIVs[&Phi][BB]; | |
| Succ->phis(), [&](PHINode &Phi) { | |
| return EBW->PhiPredIVs[&Phi][BB]; |
The explicit captures here look more confusing than helpful.
| BasicBlock *KeepBB = (*It)->BB; | ||
| BasicBlock *DeadBB = EBW.BB; | ||
|
|
||
| // Avoid merging if either is the other's predecessor in weird ways. |
Contributor
There was a problem hiding this comment.
Is this really what this condition is guarding against? I'd have thought this would occur if the same block occurs multiple times in the candidate list, rather than some kind of relationship between the blocks.
Contributor
Author
There was a problem hiding this comment.
I am sorry I forgot the context of this comment due to some rebases, but it is now just a useless comment.
| SmallSetVector<BasicBlock *, 16> FilteredArms( | ||
| llvm::from_range, | ||
| make_filter_range(successors(SI), EqualBBWrapper::canBeMerged)); | ||
| return mergeIdenticalBBs(FilteredArms.getArrayRef(), DTU); |
Contributor
There was a problem hiding this comment.
Can you please explain why we still need the special switch handling next to the predecessor based logic?
Contributor
Author
There was a problem hiding this comment.
They perform at different stages.
- "switch arm merging": very early
simplifycfg - "identical preds merging": late
simplifycfg<sink-common-inst>aftersimplifycfg, see [SimplifyCFG] Simplify identical predecessors #173022 (comment)
I indeed tried completely replacing "switch arm merging" with "identical pred merging" to consider unification; however, it triggered a regression due to the delay of "switch arm merging" (implemented by "identical pred merging").
The regression chain is too long to clarify here: dtcxzyw/llvm-opt-benchmark#3204 (comment)
Finally, I decided to keep the original logic of "switch arm merging" and append "identical pred merging" as new functionality.
Contributor
Author
|
Gentle ping😀. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/20839 Here is the relevant piece of the build log for the reference |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
When >1 predecessors of BB are identical, try to merge them into ONE.
Here is a simplified example (
sinkandbb*s share the same predecessorentry, hindering the existing uncond br folding to optimize such a case):Actually,
simplifyDuplicateSwitchArmsdid similar things in a very limited scope (only for switch arms); this patch generalizes its logic to handle any BB with >1 identical predecessors.This PR lands the discussion, i.e., "merge identical predecessor bottom to up", and implements the suggestion of #114262 (comment).