Skip to content

[DomTreeUpdater] Handle critical edge splitting #100856

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

Closed
wants to merge 1 commit into from
Closed

[DomTreeUpdater] Handle critical edge splitting #100856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions llvm/include/llvm/Analysis/DomTreeUpdater.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ class DomTreeUpdater

///@}

/// Debug method to help view the internal state of this class.
LLVM_DUMP_METHOD void dump() const;

private:
class CallBackOnDeletion final : public CallbackVH {
public:
Expand Down Expand Up @@ -111,9 +114,6 @@ class DomTreeUpdater

/// Returns true if at least one BasicBlock is deleted.
bool forceFlushDeletedBB();

/// Debug method to help view the internal state of this class.
LLVM_DUMP_METHOD void dump() const;
};

extern template class GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
Expand Down
53 changes: 53 additions & 0 deletions llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ class GenericDomTreeUpdater {
return PendUpdates.size() != PendPDTUpdateIndex;
}

bool hasPendingCriticalEdges() const {
if (!PDT && !DT)
return false;
return !CriticalEdgesToSplit.empty();
}

///@{
/// \name Mutation APIs
///
Expand Down Expand Up @@ -146,8 +152,25 @@ class GenericDomTreeUpdater {
/// 2. It is illegal to submit any update that has already been submitted,
/// i.e., you are supposed not to insert an existent edge or delete a
/// nonexistent edge.
/// 3. This kind updates are incompatible with critical edge splitting
/// updates, call this method will apply all critical edge updates in
/// lazy mode. It is not recommended to interleave applyUpdates and
/// applyUpdatesForCriticalEdgeSplitting.
Comment on lines +155 to +158
Copy link
Member

Choose a reason for hiding this comment

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

I can't parse this sentence. Could you explain it in a bit more detail?

Specifically, it is not clear to me how to reconcile 'incompatible' with 'not recommended'. What happens if we do add edge splitting updates in eager and lazy mode?

Copy link
Contributor Author

@paperchalice paperchalice Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for my poor expression.
In lazy mode

DomTreeUpdater DTU(...); // Assume in lazy mode.
DTU.applyUpdates({...}); // Several general updates.
DTU.applyUpdatesForCriticalEdgeSplitting(...); // Is equivalent to DTU.flush(); DTU.applyUpdatesForCriticalEdgeSplitting(...);
DTU.applyUpdatesForCriticalEdgeSplitting(...); // DTU will record this kind update until flush() or applyUpdates() is called.

Because cfg::Update doesn't model the update kind for critical edge splitting.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about exposing a more general update type that captures both edge additions/deletions and critical edge splitting? Would that make sense? I'm trying to see if we can avoid adding new methods and, in turn, having to document intricacies like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From user perspective a unified update method is better, i.e. applyUpdates({{From1, To1, UpdateKind::Insert}, {From, To, New, UpdateKind::Split}...}), then cfg::Update<NodePtrT> is not enough.

Copy link
Member

Choose a reason for hiding this comment

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

then cfg::Update is not enough.

Could you explain this part? I'm not sure which API we are talking about and what the limitations are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently all update operations are based on cfg::Update. If we extend cfg::Update directly e.g. add a new kind UpdateKind::Split, then it is duplicated with two insertions and deletion, and causes some compiler warnings.

Copy link
Member

Choose a reason for hiding this comment

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

We could update cfg::Update though and add one more node ref there. I still don't understand what the issue is here:

then it is duplicated with two insertions and deletion, and causes some compiler warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A critical splitting update can be expressed as insert two new edges and delete old critical edge.
Current CFG codes rely on the dichotomy of UpdateKind, a new update kind makes old match not exhaustive.
Some CFG related codes need to be updated, e.g. LegalizeUpdates which is not easy to update, because it assumes cfg::Update only has two pointer fields...

void applyUpdates(ArrayRef<typename DomTreeT::UpdateType> Updates);

/// Apply updates that the critical edge (FromBB, ToBB) has been
/// split with NewBB.
///
/// \note Do not use this method with regular edges.
///
/// \note This kind updates are incompatible with generic updates,
/// call this method will submit all generic updates in lazy mode.
/// It is not recommended to interleave applyUpdates and
/// applyUpdatesForCriticalEdgeSplitting.
void applyUpdatesForCriticalEdgeSplitting(BasicBlockT *FromBB,
BasicBlockT *ToBB,
BasicBlockT *NewBB);

/// Submit updates to all available trees. It will also
/// 1. discard duplicated updates,
/// 2. remove invalid updates. (Invalid updates means deletion of an edge that
Expand Down Expand Up @@ -197,6 +220,7 @@ class GenericDomTreeUpdater {
applyDomTreeUpdates();
applyPostDomTreeUpdates();
dropOutOfDateUpdates();
applySplitCriticalEdges();
}

///@}
Expand Down Expand Up @@ -243,6 +267,35 @@ class GenericDomTreeUpdater {
/// Drop all updates applied by all available trees and delete BasicBlocks if
/// all available trees are up-to-date.
void dropOutOfDateUpdates();

private:
/// Helper structure used to hold all the basic blocks
/// involved in the split of a critical edge.
struct CriticalEdge {
BasicBlockT *FromBB;
BasicBlockT *ToBB;
BasicBlockT *NewBB;
};

/// Pile up all the critical edges to be split.
/// The splitting of a critical edge is local and thus, it is possible
/// to apply several of those changes at the same time.
SmallVector<CriticalEdge, 32> CriticalEdgesToSplit;
Comment on lines +280 to +283
Copy link
Member

Choose a reason for hiding this comment

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

If this is local, what's the benefit of enqueuing critical edge updates vs. performing them eagerly?

Comment on lines +280 to +283
Copy link
Member

Choose a reason for hiding this comment

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

How did you pick the small size of 32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copied from abea99f.

Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to compile a few representative programs (e.g., clang) and print the maximum and average size? Otherwise I'd stick to the default small size.

Copy link
Contributor Author

@paperchalice paperchalice Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tested with bzip2:

maximum: 92
average: 8.70909

Most will not exceed 7.

Copy link
Member

@kuhar kuhar Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the due diligence! Then I'd stick to the default value


/// Remember all the basic blocks that are inserted during
/// edge splitting.
/// Invariant: NewBBs == all the basic blocks contained in the NewBB
/// field of all the elements of CriticalEdgesToSplit.
/// I.e., forall elt in CriticalEdgesToSplit, it exists BB in NewBBs
/// such as BB == elt.NewBB.
SmallSet<BasicBlockT *, 32> NewBBs;

/// Apply all the recorded critical edges to the DT.
/// This updates the underlying DT information in a way that uses
/// the fast query path of DT as much as possible.
///
/// \post CriticalEdgesToSplit.empty().
void applySplitCriticalEdges();
};

} // namespace llvm
Expand Down
143 changes: 141 additions & 2 deletions llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#ifndef LLVM_ANALYSIS_GENERICDOMTREEUPDATERIMPL_H
#define LLVM_ANALYSIS_GENERICDOMTREEUPDATERIMPL_H

#include "llvm/ADT/SmallBitVector.h"
#include "llvm/Analysis/GenericDomTreeUpdater.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_ostream.h"
Expand Down Expand Up @@ -61,6 +62,9 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
return;

if (Strategy == UpdateStrategy::Lazy) {
if (!CriticalEdgesToSplit.empty())
applySplitCriticalEdges();

PendUpdates.reserve(PendUpdates.size() + Updates.size());
for (const auto &U : Updates)
if (!isSelfDominance(U))
Expand Down Expand Up @@ -136,6 +140,7 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::getDomTree() {
assert(DT && "Invalid acquisition of a null DomTree");
applyDomTreeUpdates();
dropOutOfDateUpdates();
applySplitCriticalEdges();
return *DT;
}

Expand All @@ -145,6 +150,7 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::getPostDomTree() {
assert(PDT && "Invalid acquisition of a null PostDomTree");
applyPostDomTreeUpdates();
dropOutOfDateUpdates();
applySplitCriticalEdges();
return *PDT;
}

Expand Down Expand Up @@ -201,7 +207,7 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
if (To) {
auto S = To->getName();
if (!To->hasName())
S = "(no_name)";
S = "(no name)";
OS << S << "(" << To << ")\n";
} else {
OS << "(badref)\n";
Expand Down Expand Up @@ -229,6 +235,22 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
printUpdates(I, PendUpdates.end());
}

auto printCriticalEdges = [&](const CriticalEdge &E) {
auto FromName = E.FromBB->getName();
if (!E.FromBB->hasName())
FromName = "(no name)";
auto NewName = E.NewBB->getName();
if (!E.NewBB->hasName())
NewName = "(no name)";
auto ToName = E.ToBB->getName();
if (!E.ToBB->hasName())
ToName = "(no name)";
OS << " " << FromName << ", " << NewName << ", " << ToName << '\n';
};
OS << "Critical edges to be split:\n";
for (const auto &E : CriticalEdgesToSplit)
printCriticalEdges(E);

OS << "Pending DeletedBBs:\n";
Index = 0;
for (const auto *BB : DeletedBBs) {
Expand All @@ -237,7 +259,7 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
if (BB->hasName())
OS << BB->getName() << "(";
else
OS << "(no_name)(";
OS << "(no name)(";
OS << BB << ")\n";
}
#endif
Expand Down Expand Up @@ -348,6 +370,123 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT,
PendPDTUpdateIndex -= dropIndex;
}

template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
applyUpdatesForCriticalEdgeSplitting(BasicBlockT *FromBB, BasicBlockT *ToBB,
BasicBlockT *NewBB) {
if (!DT && !PDT)
return;
CriticalEdgesToSplit.push_back({FromBB, ToBB, NewBB});
bool Inserted = NewBBs.insert(NewBB).second;
(void)Inserted;
assert(Inserted &&
"A basic block inserted via edge splitting cannot appear twice");
if (Strategy == UpdateStrategy::Lazy) {
applyDomTreeUpdates();
applyPostDomTreeUpdates();
}
if (Strategy == UpdateStrategy::Eager)
applySplitCriticalEdges();
}

template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
void GenericDomTreeUpdater<DerivedT, DomTreeT,
PostDomTreeT>::applySplitCriticalEdges() {
// Bail out early if there is nothing to do.
if (CriticalEdgesToSplit.empty())
return;

// For each element in CriticalEdgesToSplit, remember whether or not element
// is the new immediate domminator of its successor. The mapping is done by
// index, i.e., the information for the ith element of CriticalEdgesToSplit is
// the ith element of IsNewIDom.
SmallBitVector IsNewIDom(CriticalEdgesToSplit.size(), true);
SmallBitVector IsNewIPDom(CriticalEdgesToSplit.size(), true);
size_t Idx = 0;

// Collect all the dominance properties info, before invalidating
// the underlying DT.
for (CriticalEdge &Edge : CriticalEdgesToSplit) {
// Update dominator information.
if (DT) {
BasicBlockT *Succ = Edge.ToBB;
auto *SuccDTNode = DT->getNode(Succ);

for (BasicBlockT *PredBB : predecessors(Succ)) {
if (PredBB == Edge.NewBB)
continue;
// If we are in this situation:
// FromBB1 FromBB2
// + +
// + + + +
// + + + +
// ... Split1 Split2 ...
// + +
// + +
// +
// Succ
// Instead of checking the domiance property with Split2, we check it
// with FromBB2 since Split2 is still unknown of the underlying DT
// structure.
if (NewBBs.count(PredBB)) {
assert(pred_size(PredBB) == 1 && "A basic block resulting from a "
"critical edge split has more "
"than one predecessor!");
PredBB = *pred_begin(PredBB);
}
if (!DT->dominates(SuccDTNode, DT->getNode(PredBB))) {
IsNewIDom[Idx] = false;
break;
}
}
}

// Same as DT version but from another direction.
if (PDT) {
BasicBlockT *Pred = Edge.FromBB;
auto *PredDTNode = PDT->getNode(Pred);
for (BasicBlockT *SuccBB : successors(Pred)) {
if (SuccBB == Edge.NewBB)
continue;
if (NewBBs.count(SuccBB)) {
assert(succ_size(SuccBB) == 1 && "A basic block resulting from a "
"critical edge split has more "
"than one predecessor!");
SuccBB = *succ_begin(SuccBB);
}
if (!PDT->dominates(PredDTNode, PDT->getNode(SuccBB))) {
IsNewIPDom[Idx] = false;
break;
}
}
}
++Idx;
}

// Now, update DT with the collected dominance properties info.
Idx = 0;
for (CriticalEdge &Edge : CriticalEdgesToSplit) {
if (DT) {
// We know FromBB dominates NewBB.
auto *NewDTNode = DT->addNewBlock(Edge.NewBB, Edge.FromBB);

// If all the other predecessors of "Succ" are dominated by "Succ" itself
// then the new block is the new immediate dominator of "Succ". Otherwise,
// the new block doesn't dominate anything.
if (IsNewIDom[Idx])
DT->changeImmediateDominator(DT->getNode(Edge.ToBB), NewDTNode);
}
if (PDT) {
auto *NewPDTNode = PDT->addNewBlock(Edge.NewBB, Edge.ToBB);
if (IsNewIPDom[Idx])
PDT->changeImmediateDominator(PDT->getNode(Edge.FromBB), NewPDTNode);
}
++Idx;
}
NewBBs.clear();
CriticalEdgesToSplit.clear();
}

} // namespace llvm

#endif // LLVM_ANALYSIS_GENERICDOMTREEUPDATERIMPL_H
24 changes: 16 additions & 8 deletions llvm/include/llvm/CodeGen/MachineBasicBlock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
namespace llvm {

class BasicBlock;
class MachineDomTreeUpdater;
class MachineFunction;
class MCSymbol;
class ModuleSlotTracker;
Expand Down Expand Up @@ -970,15 +971,17 @@ class MachineBasicBlock
/// MachineLoopInfo, as applicable.
MachineBasicBlock *
SplitCriticalEdge(MachineBasicBlock *Succ, Pass &P,
std::vector<SparseBitVector<>> *LiveInSets = nullptr) {
return SplitCriticalEdge(Succ, &P, nullptr, LiveInSets);
std::vector<SparseBitVector<>> *LiveInSets = nullptr,
MachineDomTreeUpdater *MDTU = nullptr) {
return SplitCriticalEdge(Succ, &P, nullptr, LiveInSets, MDTU);
}

MachineBasicBlock *
SplitCriticalEdge(MachineBasicBlock *Succ,
MachineFunctionAnalysisManager &MFAM,
std::vector<SparseBitVector<>> *LiveInSets = nullptr) {
return SplitCriticalEdge(Succ, nullptr, &MFAM, LiveInSets);
std::vector<SparseBitVector<>> *LiveInSets = nullptr,
MachineDomTreeUpdater *MDTU = nullptr) {
return SplitCriticalEdge(Succ, nullptr, &MFAM, LiveInSets, MDTU);
}

/// Check if the edge between this block and the given successor \p
Expand Down Expand Up @@ -1256,10 +1259,9 @@ class MachineBasicBlock
void removePredecessor(MachineBasicBlock *Pred);

// Helper method for new pass manager migration.
MachineBasicBlock *
SplitCriticalEdge(MachineBasicBlock *Succ, Pass *P,
MachineFunctionAnalysisManager *MFAM,
std::vector<SparseBitVector<>> *LiveInSets);
MachineBasicBlock *SplitCriticalEdge(
MachineBasicBlock *Succ, Pass *P, MachineFunctionAnalysisManager *MFAM,
std::vector<SparseBitVector<>> *LiveInSets, MachineDomTreeUpdater *MDTU);
};

raw_ostream& operator<<(raw_ostream &OS, const MachineBasicBlock &MBB);
Expand Down Expand Up @@ -1341,6 +1343,12 @@ inline auto successors(const MachineBasicBlock *BB) { return BB->successors(); }
inline auto predecessors(const MachineBasicBlock *BB) {
return BB->predecessors();
}
inline auto succ_size(const MachineBasicBlock *BB) { return BB->succ_size(); }
inline auto pred_size(const MachineBasicBlock *BB) { return BB->pred_size(); }
inline auto succ_begin(const MachineBasicBlock *BB) { return BB->succ_begin(); }
inline auto pred_begin(const MachineBasicBlock *BB) { return BB->pred_begin(); }
inline auto succ_end(const MachineBasicBlock *BB) { return BB->succ_end(); }
inline auto pred_end(const MachineBasicBlock *BB) { return BB->pred_end(); }

/// MachineInstrSpan provides an interface to get an iteration range
/// containing the instruction it was initialized with, along with all
Expand Down
Loading
Loading