Skip to content

[SimplifyCFG][NFC] Improve compile time for TryToSimplifyUncondBranchFromEmptyBlock optimization. #110715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 9, 2024
Merged

[SimplifyCFG][NFC] Improve compile time for TryToSimplifyUncondBranchFromEmptyBlock optimization. #110715

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03d9ec6
[SimplifyCFG][NFC] Improve compile time for TryToSimplifyUncondBranch…
aemerson Oct 2, 2024
7a4c521
Don't unnecessarily cache.
aemerson Oct 7, 2024
1e602db
Sink SuccPreds further.
aemerson Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions llvm/lib/Transforms/Utils/Local.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,10 +1020,11 @@ static void replaceUndefValuesInPhi(PHINode *PN,
// Only when they shares a single common predecessor, return true.
// Only handles cases when BB can't be merged while its predecessors can be
// redirected.
// \p SuccPredsOut may be partially populated with the predecessors of Succ.
static bool
CanRedirectPredsOfEmptyBBToSucc(BasicBlock *BB, BasicBlock *Succ,
const SmallPtrSetImpl<BasicBlock *> &BBPreds,
const SmallPtrSetImpl<BasicBlock *> &SuccPreds,
SmallPtrSetImpl<BasicBlock *> &SuccPredsOut,
BasicBlock *&CommonPred) {

// There must be phis in BB, otherwise BB will be merged into Succ directly
Expand All @@ -1042,7 +1043,8 @@ CanRedirectPredsOfEmptyBBToSucc(BasicBlock *BB, BasicBlock *Succ,

// Get the single common predecessor of both BB and Succ. Return false
// when there are more than one common predecessors.
for (BasicBlock *SuccPred : SuccPreds) {
for (BasicBlock *SuccPred : predecessors(Succ)) {
SuccPredsOut.insert(SuccPred);
Copy link
Contributor

@nikic nikic Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part is important to short-circuit in your case? Is it the checks above or the check in this loop?

Copy link
Contributor Author

@aemerson aemerson Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was the checks in this loop.

if (BBPreds.count(SuccPred)) {
if (CommonPred)
return false;
Expand Down Expand Up @@ -1166,7 +1168,7 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
return false;

SmallPtrSet<BasicBlock *, 16> BBPreds(pred_begin(BB), pred_end(BB));
SmallPtrSet<BasicBlock *, 16> SuccPreds(pred_begin(Succ), pred_end(Succ));
SmallPtrSet<BasicBlock *, 16> SuccPreds;

// The single common predecessor of BB and Succ when BB cannot be killed
BasicBlock *CommonPred = nullptr;
Expand All @@ -1182,6 +1184,10 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
if ((!BBKillable && !BBPhisMergeable) || introduceTooManyPhiEntries(BB, Succ))
return false;

// SuccPreds may not have been fully filled by CanRedirectPredsOfEmptyBBToSucc
// so finish it here.
SuccPreds.insert(pred_begin(Succ), pred_end(Succ));
Copy link
Contributor

@nikic nikic Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you iterate over all predecessors again here anyway, I think it would be cleaner to just only construct SuccPreds here and not try to partially cache it in CanRedirectPredsOfEmptyBBToSucc. It doesn't seem like we're actually saying any work, but it makes the API contract rather confusing.

Copy link
Contributor Author

@aemerson aemerson Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I think you're right.


// Check to see if merging these blocks/phis would cause conflicts for any of
// the phi nodes in BB or Succ. If not, we can safely merge.

Expand Down
Loading