Skip to content
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.

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

Reapply "[CodeGen] Remove applySplitCriticalEdges in MachineDominatorTree (#97055)" #98446

Closed
wants to merge 5 commits into from

Conversation

paperchalice
Copy link
Contributor

This reverts commit 6a90769.
Add an extra argument to MachineBasicBlock::SplitCriticalEdge so it can update dominator tree lazily. Now all passes that call this method and want to preserve dominator tree should pass a MachineDomTreeUpdater.
Not sure the performance hit from this is acceptable now...

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 11, 2024

@llvm/pr-subscribers-llvm-regalloc
@llvm/pr-subscribers-backend-hexagon
@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-backend-amdgpu

Author: None (paperchalice)

Changes

This reverts commit 6a90769.
Add an extra argument to MachineBasicBlock::SplitCriticalEdge so it can update dominator tree lazily. Now all passes that call this method and want to preserve dominator tree should pass a MachineDomTreeUpdater.
Not sure the performance hit from this is acceptable now...


Patch is 32.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/98446.diff

23 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+3-1)
  • (modified) llvm/include/llvm/CodeGen/MachineDominators.h (+2-167)
  • (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (+2-2)
  • (modified) llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp (+2-2)
  • (modified) llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/MachineBasicBlock.cpp (+10-6)
  • (modified) llvm/lib/CodeGen/MachineDominanceFrontier.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/MachineDominators.cpp (-74)
  • (modified) llvm/lib/CodeGen/MachineLICM.cpp (+8-1)
  • (modified) llvm/lib/CodeGen/MachineLoopInfo.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/MachineSink.cpp (+5-1)
  • (modified) llvm/lib/CodeGen/MachineUniformityAnalysis.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/PHIElimination.cpp (+17-9)
  • (modified) llvm/lib/CodeGen/XRayInstrumentation.cpp (+2-2)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.cpp (+2-3)
  • (modified) llvm/lib/Target/AMDGPU/SILateBranchLowering.cpp (+3-3)
  • (modified) llvm/lib/Target/AMDGPU/SILowerI1Copies.cpp (+1-1)
  • (modified) llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp (+1-1)
  • (modified) llvm/lib/Target/Hexagon/HexagonFrameLowering.cpp (+1-1)
  • (modified) llvm/lib/Target/X86/X86FlagsCopyLowering.cpp (+1-2)
  • (modified) llvm/tools/llvm-reduce/deltas/ReduceInstructionsMIR.cpp (+1-1)
  • (modified) llvm/unittests/Target/WebAssembly/WebAssemblyExceptionInfoTest.cpp (+4-4)
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 562d37ef32f54..88fac5e7995b1 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -33,6 +33,7 @@ namespace llvm {
 
 class BasicBlock;
 class MachineFunction;
+class MachineDomTreeUpdater;
 class MCSymbol;
 class ModuleSlotTracker;
 class Pass;
@@ -968,7 +969,8 @@ class MachineBasicBlock
   /// MachineLoopInfo, as applicable.
   MachineBasicBlock *
   SplitCriticalEdge(MachineBasicBlock *Succ, Pass &P,
-                    std::vector<SparseBitVector<>> *LiveInSets = nullptr);
+                    std::vector<SparseBitVector<>> *LiveInSets = nullptr,
+                    MachineDomTreeUpdater *MDTU = nullptr);
 
   /// Check if the edge between this block and the given successor \p
   /// Succ, can be split. If this returns true a subsequent call to
diff --git a/llvm/include/llvm/CodeGen/MachineDominators.h b/llvm/include/llvm/CodeGen/MachineDominators.h
index 74cf94398736d..61635ff64502d 100644
--- a/llvm/include/llvm/CodeGen/MachineDominators.h
+++ b/llvm/include/llvm/CodeGen/MachineDominators.h
@@ -73,86 +73,22 @@ extern template bool Verify<MBBDomTree>(const MBBDomTree &DT,
 /// compute a normal dominator tree.
 ///
 class MachineDominatorTree : public DomTreeBase<MachineBasicBlock> {
-  /// Helper structure used to hold all the basic blocks
-  /// involved in the split of a critical edge.
-  struct CriticalEdge {
-    MachineBasicBlock *FromBB;
-    MachineBasicBlock *ToBB;
-    MachineBasicBlock *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.
-  mutable SmallVector<CriticalEdge, 32> CriticalEdgesToSplit;
-
-  /// 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.
-  mutable SmallSet<MachineBasicBlock *, 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.
-  /// FIXME: This method should not be a const member!
-  ///
-  /// \post CriticalEdgesToSplit.empty().
-  void applySplitCriticalEdges() const;
 
 public:
   using Base = DomTreeBase<MachineBasicBlock>;
 
   MachineDominatorTree() = default;
-  explicit MachineDominatorTree(MachineFunction &MF) { calculate(MF); }
+  explicit MachineDominatorTree(MachineFunction &MF) { recalculate(MF); }
 
   /// Handle invalidation explicitly.
   bool invalidate(MachineFunction &, const PreservedAnalyses &PA,
                   MachineFunctionAnalysisManager::Invalidator &);
 
-  // FIXME: If there is an updater for MachineDominatorTree,
-  // migrate to this updater and remove these wrappers.
-
-  MachineDominatorTree &getBase() {
-    applySplitCriticalEdges();
-    return *this;
-  }
-
-  MachineBasicBlock *getRoot() const {
-    applySplitCriticalEdges();
-    return Base::getRoot();
-  }
-
-  MachineDomTreeNode *getRootNode() const {
-    applySplitCriticalEdges();
-    return const_cast<MachineDomTreeNode *>(Base::getRootNode());
-  }
-
-  void calculate(MachineFunction &F);
-
-  bool dominates(const MachineDomTreeNode *A,
-                 const MachineDomTreeNode *B) const {
-    applySplitCriticalEdges();
-    return Base::dominates(A, B);
-  }
-
-  void getDescendants(MachineBasicBlock *A,
-                      SmallVectorImpl<MachineBasicBlock *> &Result) {
-    applySplitCriticalEdges();
-    Base::getDescendants(A, Result);
-  }
-
-  bool dominates(const MachineBasicBlock *A, const MachineBasicBlock *B) const {
-    applySplitCriticalEdges();
-    return Base::dominates(A, B);
-  }
+  using Base::dominates;
 
   // dominates - Return true if A dominates B. This performs the
   // special checks necessary if A and B are in the same basic block.
   bool dominates(const MachineInstr *A, const MachineInstr *B) const {
-    applySplitCriticalEdges();
     const MachineBasicBlock *BBA = A->getParent(), *BBB = B->getParent();
     if (BBA != BBB)
       return Base::dominates(BBA, BBB);
@@ -164,107 +100,6 @@ class MachineDominatorTree : public DomTreeBase<MachineBasicBlock> {
 
     return &*I == A;
   }
-
-  bool properlyDominates(const MachineDomTreeNode *A,
-                         const MachineDomTreeNode *B) const {
-    applySplitCriticalEdges();
-    return Base::properlyDominates(A, B);
-  }
-
-  bool properlyDominates(const MachineBasicBlock *A,
-                         const MachineBasicBlock *B) const {
-    applySplitCriticalEdges();
-    return Base::properlyDominates(A, B);
-  }
-
-  /// findNearestCommonDominator - Find nearest common dominator basic block
-  /// for basic block A and B. If there is no such block then return NULL.
-  MachineBasicBlock *findNearestCommonDominator(MachineBasicBlock *A,
-                                                MachineBasicBlock *B) {
-    applySplitCriticalEdges();
-    return Base::findNearestCommonDominator(A, B);
-  }
-
-  MachineDomTreeNode *operator[](MachineBasicBlock *BB) const {
-    applySplitCriticalEdges();
-    return Base::getNode(BB);
-  }
-
-  /// getNode - return the (Post)DominatorTree node for the specified basic
-  /// block.  This is the same as using operator[] on this class.
-  ///
-  MachineDomTreeNode *getNode(MachineBasicBlock *BB) const {
-    applySplitCriticalEdges();
-    return Base::getNode(BB);
-  }
-
-  /// addNewBlock - Add a new node to the dominator tree information.  This
-  /// creates a new node as a child of DomBB dominator node,linking it into
-  /// the children list of the immediate dominator.
-  MachineDomTreeNode *addNewBlock(MachineBasicBlock *BB,
-                                  MachineBasicBlock *DomBB) {
-    applySplitCriticalEdges();
-    return Base::addNewBlock(BB, DomBB);
-  }
-
-  /// changeImmediateDominator - This method is used to update the dominator
-  /// tree information when a node's immediate dominator changes.
-  ///
-  void changeImmediateDominator(MachineBasicBlock *N,
-                                MachineBasicBlock *NewIDom) {
-    applySplitCriticalEdges();
-    Base::changeImmediateDominator(N, NewIDom);
-  }
-
-  void changeImmediateDominator(MachineDomTreeNode *N,
-                                MachineDomTreeNode *NewIDom) {
-    applySplitCriticalEdges();
-    Base::changeImmediateDominator(N, NewIDom);
-  }
-
-  /// eraseNode - Removes a node from  the dominator tree. Block must not
-  /// dominate any other blocks. Removes node from its immediate dominator's
-  /// children list. Deletes dominator node associated with basic block BB.
-  void eraseNode(MachineBasicBlock *BB) {
-    applySplitCriticalEdges();
-    Base::eraseNode(BB);
-  }
-
-  /// splitBlock - BB is split and now it has one successor. Update dominator
-  /// tree to reflect this change.
-  void splitBlock(MachineBasicBlock* NewBB) {
-    applySplitCriticalEdges();
-    Base::splitBlock(NewBB);
-  }
-
-  /// isReachableFromEntry - Return true if A is dominated by the entry
-  /// block of the function containing it.
-  bool isReachableFromEntry(const MachineBasicBlock *A) {
-    applySplitCriticalEdges();
-    return Base::isReachableFromEntry(A);
-  }
-
-  /// Record that the critical edge (FromBB, ToBB) has been
-  /// split with NewBB.
-  /// This is best to use this method instead of directly update the
-  /// underlying information, because this helps mitigating the
-  /// number of time the DT information is invalidated.
-  ///
-  /// \note Do not use this method with regular edges.
-  ///
-  /// \note To benefit from the compile time improvement incurred by this
-  /// method, the users of this method have to limit the queries to the DT
-  /// interface between two edges splitting. In other words, they have to
-  /// pack the splitting of critical edges as much as possible.
-  void recordSplitCriticalEdge(MachineBasicBlock *FromBB,
-                              MachineBasicBlock *ToBB,
-                              MachineBasicBlock *NewBB) {
-    bool Inserted = NewBBs.insert(NewBB).second;
-    (void)Inserted;
-    assert(Inserted &&
-           "A basic block inserted via edge splitting cannot appear twice");
-    CriticalEdgesToSplit.push_back({FromBB, ToBB, NewBB});
-  }
 };
 
 /// \brief Analysis pass which computes a \c MachineDominatorTree.
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 1f59ec545b4f7..c8da52715fb1f 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1698,7 +1698,7 @@ void AsmPrinter::emitFunctionBody() {
     MDT = MDTWrapper ? &MDTWrapper->getDomTree() : nullptr;
     if (!MDT) {
       OwnedMDT = std::make_unique<MachineDominatorTree>();
-      OwnedMDT->getBase().recalculate(*MF);
+      OwnedMDT->recalculate(*MF);
       MDT = OwnedMDT.get();
     }
 
@@ -1707,7 +1707,7 @@ void AsmPrinter::emitFunctionBody() {
     MLI = MLIWrapper ? &MLIWrapper->getLI() : nullptr;
     if (!MLI) {
       OwnedMLI = std::make_unique<MachineLoopInfo>();
-      OwnedMLI->analyze(MDT->getBase());
+      OwnedMLI->analyze(*MDT);
       MLI = OwnedMLI.get();
     }
   }
diff --git a/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp b/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp
index 0cf01edbff6c8..caaaf7e466f28 100644
--- a/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp
+++ b/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp
@@ -78,13 +78,13 @@ LazyMachineBlockFrequencyInfoPass::calculateIfNotAvailable() const {
     if (!MDT) {
       LLVM_DEBUG(dbgs() << "Building DominatorTree on the fly\n");
       OwnedMDT = std::make_unique<MachineDominatorTree>();
-      OwnedMDT->getBase().recalculate(*MF);
+      OwnedMDT->recalculate(*MF);
       MDT = OwnedMDT.get();
     }
 
     // Generate LoopInfo from it.
     OwnedMLI = std::make_unique<MachineLoopInfo>();
-    OwnedMLI->analyze(MDT->getBase());
+    OwnedMLI->analyze(*MDT);
     MLI = OwnedMLI.get();
   }
 
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index 555cbb7a507f4..98099ace804dd 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -2754,7 +2754,7 @@ void InstrRefBasedLDV::BlockPHIPlacement(
   // Apply IDF calculator to the designated set of location defs, storing
   // required PHIs into PHIBlocks. Uses the dominator tree stored in the
   // InstrRefBasedLDV object.
-  IDFCalculatorBase<MachineBasicBlock, false> IDF(DomTree->getBase());
+  IDFCalculatorBase<MachineBasicBlock, false> IDF(*DomTree);
 
   IDF.setLiveInBlocks(AllBlocks);
   IDF.setDefiningBlocks(DefBlocks);
diff --git a/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp b/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
index 0c0a4e13c7c9e..15642969457aa 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
@@ -120,7 +120,7 @@ bool LiveDebugValues::runOnMachineFunction(MachineFunction &MF) {
   MachineDominatorTree *DomTree = nullptr;
   if (InstrRefBased) {
     DomTree = &MDT;
-    MDT.calculate(MF);
+    MDT.recalculate(MF);
     TheImpl = &*InstrRefImpl;
   }
 
diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp
index 5fe7a9d35dc9a..bd81b068046f7 100644
--- a/llvm/lib/CodeGen/MachineBasicBlock.cpp
+++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp
@@ -16,11 +16,13 @@
 #include "llvm/CodeGen/LiveIntervals.h"
 #include "llvm/CodeGen/LivePhysRegs.h"
 #include "llvm/CodeGen/LiveVariables.h"
+#include "llvm/CodeGen/MachineDomTreeUpdater.h"
 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineJumpTableInfo.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
+#include "llvm/CodeGen/MachinePostDominators.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/SlotIndexes.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
@@ -1135,9 +1137,10 @@ class SlotIndexUpdateDelegate : public MachineFunction::Delegate {
   }
 };
 
-MachineBasicBlock *MachineBasicBlock::SplitCriticalEdge(
-    MachineBasicBlock *Succ, Pass &P,
-    std::vector<SparseBitVector<>> *LiveInSets) {
+MachineBasicBlock *
+MachineBasicBlock::SplitCriticalEdge(MachineBasicBlock *Succ, Pass &P,
+                                     std::vector<SparseBitVector<>> *LiveInSets,
+                                     MachineDomTreeUpdater *MDTU) {
   if (!canSplitCriticalEdge(Succ))
     return nullptr;
 
@@ -1339,9 +1342,10 @@ MachineBasicBlock *MachineBasicBlock::SplitCriticalEdge(
     LIS->repairIntervalsInRange(this, getFirstTerminator(), end(), UsedRegs);
   }
 
-  if (auto *MDTWrapper =
-          P.getAnalysisIfAvailable<MachineDominatorTreeWrapperPass>())
-    MDTWrapper->getDomTree().recordSplitCriticalEdge(this, Succ, NMBB);
+  if (MDTU)
+    MDTU->applyUpdates({{MachineDominatorTree::Insert, this, NMBB},
+                        {MachineDominatorTree::Insert, NMBB, Succ},
+                        {MachineDominatorTree::Delete, this, Succ}});
 
   auto *MLIWrapper = P.getAnalysisIfAvailable<MachineLoopInfoWrapperPass>();
   if (MachineLoopInfo *MLI = MLIWrapper ? &MLIWrapper->getLI() : nullptr)
diff --git a/llvm/lib/CodeGen/MachineDominanceFrontier.cpp b/llvm/lib/CodeGen/MachineDominanceFrontier.cpp
index 6a8ede4feb937..ed69ed931c5cb 100644
--- a/llvm/lib/CodeGen/MachineDominanceFrontier.cpp
+++ b/llvm/lib/CodeGen/MachineDominanceFrontier.cpp
@@ -38,8 +38,7 @@ char &llvm::MachineDominanceFrontierID = MachineDominanceFrontier::ID;
 
 bool MachineDominanceFrontier::runOnMachineFunction(MachineFunction &) {
   releaseMemory();
-  Base.analyze(
-      getAnalysis<MachineDominatorTreeWrapperPass>().getDomTree().getBase());
+  Base.analyze(getAnalysis<MachineDominatorTreeWrapperPass>().getDomTree());
   return false;
 }
 
diff --git a/llvm/lib/CodeGen/MachineDominators.cpp b/llvm/lib/CodeGen/MachineDominators.cpp
index a2cc8fdfa7c9f..67a91c87bb1bc 100644
--- a/llvm/lib/CodeGen/MachineDominators.cpp
+++ b/llvm/lib/CodeGen/MachineDominators.cpp
@@ -95,12 +95,6 @@ MachineDominatorTreeWrapperPass::MachineDominatorTreeWrapperPass()
       *PassRegistry::getPassRegistry());
 }
 
-void MachineDominatorTree::calculate(MachineFunction &F) {
-  CriticalEdgesToSplit.clear();
-  NewBBs.clear();
-  recalculate(F);
-}
-
 char &llvm::MachineDominatorsID = MachineDominatorTreeWrapperPass::ID;
 
 bool MachineDominatorTreeWrapperPass::runOnMachineFunction(MachineFunction &F) {
@@ -121,71 +115,3 @@ void MachineDominatorTreeWrapperPass::print(raw_ostream &OS,
   if (DT)
     DT->print(OS);
 }
-
-void MachineDominatorTree::applySplitCriticalEdges() const {
-  // 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);
-  size_t Idx = 0;
-
-  // Collect all the dominance properties info, before invalidating
-  // the underlying DT.
-  for (CriticalEdge &Edge : CriticalEdgesToSplit) {
-    // Update dominator information.
-    MachineBasicBlock *Succ = Edge.ToBB;
-    MachineDomTreeNode *SuccDTNode = Base::getNode(Succ);
-
-    for (MachineBasicBlock *PredBB : Succ->predecessors()) {
-      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(PredBB->pred_size() == 1 && "A basic block resulting from a "
-                                           "critical edge split has more "
-                                           "than one predecessor!");
-        PredBB = *PredBB->pred_begin();
-      }
-      if (!Base::dominates(SuccDTNode, Base::getNode(PredBB))) {
-        IsNewIDom[Idx] = false;
-        break;
-      }
-    }
-    ++Idx;
-  }
-
-  // Now, update DT with the collected dominance properties info.
-  Idx = 0;
-  for (CriticalEdge &Edge : CriticalEdgesToSplit) {
-    // We know FromBB dominates NewBB.
-    MachineDomTreeNode *NewDTNode =
-        const_cast<MachineDominatorTree *>(this)->Base::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])
-      const_cast<MachineDominatorTree *>(this)->Base::changeImmediateDominator(
-          Base::getNode(Edge.ToBB), NewDTNode);
-    ++Idx;
-  }
-  NewBBs.clear();
-  CriticalEdgesToSplit.clear();
-}
diff --git a/llvm/lib/CodeGen/MachineLICM.cpp b/llvm/lib/CodeGen/MachineLICM.cpp
index 7a0c8ba081850..7fedefd76ab64 100644
--- a/llvm/lib/CodeGen/MachineLICM.cpp
+++ b/llvm/lib/CodeGen/MachineLICM.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
+#include "llvm/CodeGen/MachineDomTreeUpdater.h"
 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
@@ -123,6 +124,7 @@ namespace {
     const TargetRegisterInfo *TRI = nullptr;
     const MachineFrameInfo *MFI = nullptr;
     MachineRegisterInfo *MRI = nullptr;
+    MachineDomTreeUpdater *MDTU = nullptr;
     TargetSchedModel SchedModel;
     bool PreRegAlloc = false;
     bool HasProfileData = false;
@@ -378,6 +380,10 @@ bool MachineLICMBase::runOnMachineFunction(MachineFunction &MF) {
   DT = &getAnalysis<MachineDominatorTreeWrapperPass>().getDomTree();
   AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
 
+  MachineDomTreeUpdater Updater(DT,
+                                MachineDomTreeUpdater::UpdateStrategy::Lazy);
+  MDTU = &Updater;
+
   if (HoistConstLoads)
     InitializeLoadsHoistableLoops();
 
@@ -1704,7 +1710,8 @@ MachineLICMBase::getCurPreheader(MachineLoop *CurLoop,
         return nullptr;
       }
 
-      CurPreheader = Pred->SplitCriticalEdge(CurLoop->getHeader(), *this);
+      CurPreheader =
+          Pred->SplitCriticalEdge(CurLoop->getHeader(), *this, nullptr, MDTU);
       if (!CurPreheader) {
         CurPreheader = reinterpret_cast<MachineBasicBlock *>(-1);
         return nullptr;
diff --git a/llvm/lib/CodeGen/MachineLoopInfo.cpp b/llvm/lib/CodeGen/MachineLoopInfo.cpp
index a03c008e6045a..ea3f4fbaff479 100644
--- a/llvm/lib/CodeGen/MachineLoopInfo.cpp
+++ b/llvm/lib/CodeGen/MachineLoopInfo.cpp
@@ -77,7 +77,7 @@ bool MachineLoopInfo::invalidate(
 
 void MachineLoopInfo::calculate(MachineDominatorTree &MDT) {
   releaseMemory();
-  analyze(MDT.getBase());
+  analyze(MDT);
 }
 
 void MachineLoopInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index bbc5ab13a0cd3..a621091e62fee 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -30,6 +30,7 @@
 #include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
 #include "llvm/CodeGen/MachineBranchProbabilityInfo.h"
 #include "llvm/CodeGen/MachineCycleAnalysis.h"
+#include "llvm/CodeGen/MachineDomTreeUpdater.h"
 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
@@ -728,6 +729,8 @@ boo...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 11, 2024

@llvm/pr-subscribers-debuginfo

Author: None (paperchalice)

Changes

This reverts commit 6a90769.
Add an extra argument to MachineBasicBlock::SplitCriticalEdge so it can update dominator tree lazily. Now all passes that call this method and want to preserve dominator tree should pass a MachineDomTreeUpdater.
Not sure the performance hit from this is acceptable now...


Patch is 32.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/98446.diff

23 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+3-1)
  • (modified) llvm/include/llvm/CodeGen/MachineDominators.h (+2-167)
  • (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (+2-2)
  • (modified) llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp (+2-2)
  • (modified) llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/MachineBasicBlock.cpp (+10-6)
  • (modified) llvm/lib/CodeGen/MachineDominanceFrontier.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/MachineDominators.cpp (-74)
  • (modified) llvm/lib/CodeGen/MachineLICM.cpp (+8-1)
  • (modified) llvm/lib/CodeGen/MachineLoopInfo.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/MachineSink.cpp (+5-1)
  • (modified) llvm/lib/CodeGen/MachineUniformityAnalysis.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/PHIElimination.cpp (+17-9)
  • (modified) llvm/lib/CodeGen/XRayInstrumentation.cpp (+2-2)
  • (modified) llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.cpp (+2-3)
  • (modified) llvm/lib/Target/AMDGPU/SILateBranchLowering.cpp (+3-3)
  • (modified) llvm/lib/Target/AMDGPU/SILowerI1Copies.cpp (+1-1)
  • (modified) llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp (+1-1)
  • (modified) llvm/lib/Target/Hexagon/HexagonFrameLowering.cpp (+1-1)
  • (modified) llvm/lib/Target/X86/X86FlagsCopyLowering.cpp (+1-2)
  • (modified) llvm/tools/llvm-reduce/deltas/ReduceInstructionsMIR.cpp (+1-1)
  • (modified) llvm/unittests/Target/WebAssembly/WebAssemblyExceptionInfoTest.cpp (+4-4)
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 562d37ef32f54..88fac5e7995b1 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -33,6 +33,7 @@ namespace llvm {
 
 class BasicBlock;
 class MachineFunction;
+class MachineDomTreeUpdater;
 class MCSymbol;
 class ModuleSlotTracker;
 class Pass;
@@ -968,7 +969,8 @@ class MachineBasicBlock
   /// MachineLoopInfo, as applicable.
   MachineBasicBlock *
   SplitCriticalEdge(MachineBasicBlock *Succ, Pass &P,
-                    std::vector<SparseBitVector<>> *LiveInSets = nullptr);
+                    std::vector<SparseBitVector<>> *LiveInSets = nullptr,
+                    MachineDomTreeUpdater *MDTU = nullptr);
 
   /// Check if the edge between this block and the given successor \p
   /// Succ, can be split. If this returns true a subsequent call to
diff --git a/llvm/include/llvm/CodeGen/MachineDominators.h b/llvm/include/llvm/CodeGen/MachineDominators.h
index 74cf94398736d..61635ff64502d 100644
--- a/llvm/include/llvm/CodeGen/MachineDominators.h
+++ b/llvm/include/llvm/CodeGen/MachineDominators.h
@@ -73,86 +73,22 @@ extern template bool Verify<MBBDomTree>(const MBBDomTree &DT,
 /// compute a normal dominator tree.
 ///
 class MachineDominatorTree : public DomTreeBase<MachineBasicBlock> {
-  /// Helper structure used to hold all the basic blocks
-  /// involved in the split of a critical edge.
-  struct CriticalEdge {
-    MachineBasicBlock *FromBB;
-    MachineBasicBlock *ToBB;
-    MachineBasicBlock *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.
-  mutable SmallVector<CriticalEdge, 32> CriticalEdgesToSplit;
-
-  /// 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.
-  mutable SmallSet<MachineBasicBlock *, 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.
-  /// FIXME: This method should not be a const member!
-  ///
-  /// \post CriticalEdgesToSplit.empty().
-  void applySplitCriticalEdges() const;
 
 public:
   using Base = DomTreeBase<MachineBasicBlock>;
 
   MachineDominatorTree() = default;
-  explicit MachineDominatorTree(MachineFunction &MF) { calculate(MF); }
+  explicit MachineDominatorTree(MachineFunction &MF) { recalculate(MF); }
 
   /// Handle invalidation explicitly.
   bool invalidate(MachineFunction &, const PreservedAnalyses &PA,
                   MachineFunctionAnalysisManager::Invalidator &);
 
-  // FIXME: If there is an updater for MachineDominatorTree,
-  // migrate to this updater and remove these wrappers.
-
-  MachineDominatorTree &getBase() {
-    applySplitCriticalEdges();
-    return *this;
-  }
-
-  MachineBasicBlock *getRoot() const {
-    applySplitCriticalEdges();
-    return Base::getRoot();
-  }
-
-  MachineDomTreeNode *getRootNode() const {
-    applySplitCriticalEdges();
-    return const_cast<MachineDomTreeNode *>(Base::getRootNode());
-  }
-
-  void calculate(MachineFunction &F);
-
-  bool dominates(const MachineDomTreeNode *A,
-                 const MachineDomTreeNode *B) const {
-    applySplitCriticalEdges();
-    return Base::dominates(A, B);
-  }
-
-  void getDescendants(MachineBasicBlock *A,
-                      SmallVectorImpl<MachineBasicBlock *> &Result) {
-    applySplitCriticalEdges();
-    Base::getDescendants(A, Result);
-  }
-
-  bool dominates(const MachineBasicBlock *A, const MachineBasicBlock *B) const {
-    applySplitCriticalEdges();
-    return Base::dominates(A, B);
-  }
+  using Base::dominates;
 
   // dominates - Return true if A dominates B. This performs the
   // special checks necessary if A and B are in the same basic block.
   bool dominates(const MachineInstr *A, const MachineInstr *B) const {
-    applySplitCriticalEdges();
     const MachineBasicBlock *BBA = A->getParent(), *BBB = B->getParent();
     if (BBA != BBB)
       return Base::dominates(BBA, BBB);
@@ -164,107 +100,6 @@ class MachineDominatorTree : public DomTreeBase<MachineBasicBlock> {
 
     return &*I == A;
   }
-
-  bool properlyDominates(const MachineDomTreeNode *A,
-                         const MachineDomTreeNode *B) const {
-    applySplitCriticalEdges();
-    return Base::properlyDominates(A, B);
-  }
-
-  bool properlyDominates(const MachineBasicBlock *A,
-                         const MachineBasicBlock *B) const {
-    applySplitCriticalEdges();
-    return Base::properlyDominates(A, B);
-  }
-
-  /// findNearestCommonDominator - Find nearest common dominator basic block
-  /// for basic block A and B. If there is no such block then return NULL.
-  MachineBasicBlock *findNearestCommonDominator(MachineBasicBlock *A,
-                                                MachineBasicBlock *B) {
-    applySplitCriticalEdges();
-    return Base::findNearestCommonDominator(A, B);
-  }
-
-  MachineDomTreeNode *operator[](MachineBasicBlock *BB) const {
-    applySplitCriticalEdges();
-    return Base::getNode(BB);
-  }
-
-  /// getNode - return the (Post)DominatorTree node for the specified basic
-  /// block.  This is the same as using operator[] on this class.
-  ///
-  MachineDomTreeNode *getNode(MachineBasicBlock *BB) const {
-    applySplitCriticalEdges();
-    return Base::getNode(BB);
-  }
-
-  /// addNewBlock - Add a new node to the dominator tree information.  This
-  /// creates a new node as a child of DomBB dominator node,linking it into
-  /// the children list of the immediate dominator.
-  MachineDomTreeNode *addNewBlock(MachineBasicBlock *BB,
-                                  MachineBasicBlock *DomBB) {
-    applySplitCriticalEdges();
-    return Base::addNewBlock(BB, DomBB);
-  }
-
-  /// changeImmediateDominator - This method is used to update the dominator
-  /// tree information when a node's immediate dominator changes.
-  ///
-  void changeImmediateDominator(MachineBasicBlock *N,
-                                MachineBasicBlock *NewIDom) {
-    applySplitCriticalEdges();
-    Base::changeImmediateDominator(N, NewIDom);
-  }
-
-  void changeImmediateDominator(MachineDomTreeNode *N,
-                                MachineDomTreeNode *NewIDom) {
-    applySplitCriticalEdges();
-    Base::changeImmediateDominator(N, NewIDom);
-  }
-
-  /// eraseNode - Removes a node from  the dominator tree. Block must not
-  /// dominate any other blocks. Removes node from its immediate dominator's
-  /// children list. Deletes dominator node associated with basic block BB.
-  void eraseNode(MachineBasicBlock *BB) {
-    applySplitCriticalEdges();
-    Base::eraseNode(BB);
-  }
-
-  /// splitBlock - BB is split and now it has one successor. Update dominator
-  /// tree to reflect this change.
-  void splitBlock(MachineBasicBlock* NewBB) {
-    applySplitCriticalEdges();
-    Base::splitBlock(NewBB);
-  }
-
-  /// isReachableFromEntry - Return true if A is dominated by the entry
-  /// block of the function containing it.
-  bool isReachableFromEntry(const MachineBasicBlock *A) {
-    applySplitCriticalEdges();
-    return Base::isReachableFromEntry(A);
-  }
-
-  /// Record that the critical edge (FromBB, ToBB) has been
-  /// split with NewBB.
-  /// This is best to use this method instead of directly update the
-  /// underlying information, because this helps mitigating the
-  /// number of time the DT information is invalidated.
-  ///
-  /// \note Do not use this method with regular edges.
-  ///
-  /// \note To benefit from the compile time improvement incurred by this
-  /// method, the users of this method have to limit the queries to the DT
-  /// interface between two edges splitting. In other words, they have to
-  /// pack the splitting of critical edges as much as possible.
-  void recordSplitCriticalEdge(MachineBasicBlock *FromBB,
-                              MachineBasicBlock *ToBB,
-                              MachineBasicBlock *NewBB) {
-    bool Inserted = NewBBs.insert(NewBB).second;
-    (void)Inserted;
-    assert(Inserted &&
-           "A basic block inserted via edge splitting cannot appear twice");
-    CriticalEdgesToSplit.push_back({FromBB, ToBB, NewBB});
-  }
 };
 
 /// \brief Analysis pass which computes a \c MachineDominatorTree.
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 1f59ec545b4f7..c8da52715fb1f 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1698,7 +1698,7 @@ void AsmPrinter::emitFunctionBody() {
     MDT = MDTWrapper ? &MDTWrapper->getDomTree() : nullptr;
     if (!MDT) {
       OwnedMDT = std::make_unique<MachineDominatorTree>();
-      OwnedMDT->getBase().recalculate(*MF);
+      OwnedMDT->recalculate(*MF);
       MDT = OwnedMDT.get();
     }
 
@@ -1707,7 +1707,7 @@ void AsmPrinter::emitFunctionBody() {
     MLI = MLIWrapper ? &MLIWrapper->getLI() : nullptr;
     if (!MLI) {
       OwnedMLI = std::make_unique<MachineLoopInfo>();
-      OwnedMLI->analyze(MDT->getBase());
+      OwnedMLI->analyze(*MDT);
       MLI = OwnedMLI.get();
     }
   }
diff --git a/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp b/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp
index 0cf01edbff6c8..caaaf7e466f28 100644
--- a/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp
+++ b/llvm/lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp
@@ -78,13 +78,13 @@ LazyMachineBlockFrequencyInfoPass::calculateIfNotAvailable() const {
     if (!MDT) {
       LLVM_DEBUG(dbgs() << "Building DominatorTree on the fly\n");
       OwnedMDT = std::make_unique<MachineDominatorTree>();
-      OwnedMDT->getBase().recalculate(*MF);
+      OwnedMDT->recalculate(*MF);
       MDT = OwnedMDT.get();
     }
 
     // Generate LoopInfo from it.
     OwnedMLI = std::make_unique<MachineLoopInfo>();
-    OwnedMLI->analyze(MDT->getBase());
+    OwnedMLI->analyze(*MDT);
     MLI = OwnedMLI.get();
   }
 
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index 555cbb7a507f4..98099ace804dd 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -2754,7 +2754,7 @@ void InstrRefBasedLDV::BlockPHIPlacement(
   // Apply IDF calculator to the designated set of location defs, storing
   // required PHIs into PHIBlocks. Uses the dominator tree stored in the
   // InstrRefBasedLDV object.
-  IDFCalculatorBase<MachineBasicBlock, false> IDF(DomTree->getBase());
+  IDFCalculatorBase<MachineBasicBlock, false> IDF(*DomTree);
 
   IDF.setLiveInBlocks(AllBlocks);
   IDF.setDefiningBlocks(DefBlocks);
diff --git a/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp b/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
index 0c0a4e13c7c9e..15642969457aa 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
@@ -120,7 +120,7 @@ bool LiveDebugValues::runOnMachineFunction(MachineFunction &MF) {
   MachineDominatorTree *DomTree = nullptr;
   if (InstrRefBased) {
     DomTree = &MDT;
-    MDT.calculate(MF);
+    MDT.recalculate(MF);
     TheImpl = &*InstrRefImpl;
   }
 
diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp
index 5fe7a9d35dc9a..bd81b068046f7 100644
--- a/llvm/lib/CodeGen/MachineBasicBlock.cpp
+++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp
@@ -16,11 +16,13 @@
 #include "llvm/CodeGen/LiveIntervals.h"
 #include "llvm/CodeGen/LivePhysRegs.h"
 #include "llvm/CodeGen/LiveVariables.h"
+#include "llvm/CodeGen/MachineDomTreeUpdater.h"
 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineJumpTableInfo.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
+#include "llvm/CodeGen/MachinePostDominators.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/SlotIndexes.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
@@ -1135,9 +1137,10 @@ class SlotIndexUpdateDelegate : public MachineFunction::Delegate {
   }
 };
 
-MachineBasicBlock *MachineBasicBlock::SplitCriticalEdge(
-    MachineBasicBlock *Succ, Pass &P,
-    std::vector<SparseBitVector<>> *LiveInSets) {
+MachineBasicBlock *
+MachineBasicBlock::SplitCriticalEdge(MachineBasicBlock *Succ, Pass &P,
+                                     std::vector<SparseBitVector<>> *LiveInSets,
+                                     MachineDomTreeUpdater *MDTU) {
   if (!canSplitCriticalEdge(Succ))
     return nullptr;
 
@@ -1339,9 +1342,10 @@ MachineBasicBlock *MachineBasicBlock::SplitCriticalEdge(
     LIS->repairIntervalsInRange(this, getFirstTerminator(), end(), UsedRegs);
   }
 
-  if (auto *MDTWrapper =
-          P.getAnalysisIfAvailable<MachineDominatorTreeWrapperPass>())
-    MDTWrapper->getDomTree().recordSplitCriticalEdge(this, Succ, NMBB);
+  if (MDTU)
+    MDTU->applyUpdates({{MachineDominatorTree::Insert, this, NMBB},
+                        {MachineDominatorTree::Insert, NMBB, Succ},
+                        {MachineDominatorTree::Delete, this, Succ}});
 
   auto *MLIWrapper = P.getAnalysisIfAvailable<MachineLoopInfoWrapperPass>();
   if (MachineLoopInfo *MLI = MLIWrapper ? &MLIWrapper->getLI() : nullptr)
diff --git a/llvm/lib/CodeGen/MachineDominanceFrontier.cpp b/llvm/lib/CodeGen/MachineDominanceFrontier.cpp
index 6a8ede4feb937..ed69ed931c5cb 100644
--- a/llvm/lib/CodeGen/MachineDominanceFrontier.cpp
+++ b/llvm/lib/CodeGen/MachineDominanceFrontier.cpp
@@ -38,8 +38,7 @@ char &llvm::MachineDominanceFrontierID = MachineDominanceFrontier::ID;
 
 bool MachineDominanceFrontier::runOnMachineFunction(MachineFunction &) {
   releaseMemory();
-  Base.analyze(
-      getAnalysis<MachineDominatorTreeWrapperPass>().getDomTree().getBase());
+  Base.analyze(getAnalysis<MachineDominatorTreeWrapperPass>().getDomTree());
   return false;
 }
 
diff --git a/llvm/lib/CodeGen/MachineDominators.cpp b/llvm/lib/CodeGen/MachineDominators.cpp
index a2cc8fdfa7c9f..67a91c87bb1bc 100644
--- a/llvm/lib/CodeGen/MachineDominators.cpp
+++ b/llvm/lib/CodeGen/MachineDominators.cpp
@@ -95,12 +95,6 @@ MachineDominatorTreeWrapperPass::MachineDominatorTreeWrapperPass()
       *PassRegistry::getPassRegistry());
 }
 
-void MachineDominatorTree::calculate(MachineFunction &F) {
-  CriticalEdgesToSplit.clear();
-  NewBBs.clear();
-  recalculate(F);
-}
-
 char &llvm::MachineDominatorsID = MachineDominatorTreeWrapperPass::ID;
 
 bool MachineDominatorTreeWrapperPass::runOnMachineFunction(MachineFunction &F) {
@@ -121,71 +115,3 @@ void MachineDominatorTreeWrapperPass::print(raw_ostream &OS,
   if (DT)
     DT->print(OS);
 }
-
-void MachineDominatorTree::applySplitCriticalEdges() const {
-  // 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);
-  size_t Idx = 0;
-
-  // Collect all the dominance properties info, before invalidating
-  // the underlying DT.
-  for (CriticalEdge &Edge : CriticalEdgesToSplit) {
-    // Update dominator information.
-    MachineBasicBlock *Succ = Edge.ToBB;
-    MachineDomTreeNode *SuccDTNode = Base::getNode(Succ);
-
-    for (MachineBasicBlock *PredBB : Succ->predecessors()) {
-      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(PredBB->pred_size() == 1 && "A basic block resulting from a "
-                                           "critical edge split has more "
-                                           "than one predecessor!");
-        PredBB = *PredBB->pred_begin();
-      }
-      if (!Base::dominates(SuccDTNode, Base::getNode(PredBB))) {
-        IsNewIDom[Idx] = false;
-        break;
-      }
-    }
-    ++Idx;
-  }
-
-  // Now, update DT with the collected dominance properties info.
-  Idx = 0;
-  for (CriticalEdge &Edge : CriticalEdgesToSplit) {
-    // We know FromBB dominates NewBB.
-    MachineDomTreeNode *NewDTNode =
-        const_cast<MachineDominatorTree *>(this)->Base::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])
-      const_cast<MachineDominatorTree *>(this)->Base::changeImmediateDominator(
-          Base::getNode(Edge.ToBB), NewDTNode);
-    ++Idx;
-  }
-  NewBBs.clear();
-  CriticalEdgesToSplit.clear();
-}
diff --git a/llvm/lib/CodeGen/MachineLICM.cpp b/llvm/lib/CodeGen/MachineLICM.cpp
index 7a0c8ba081850..7fedefd76ab64 100644
--- a/llvm/lib/CodeGen/MachineLICM.cpp
+++ b/llvm/lib/CodeGen/MachineLICM.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
+#include "llvm/CodeGen/MachineDomTreeUpdater.h"
 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
@@ -123,6 +124,7 @@ namespace {
     const TargetRegisterInfo *TRI = nullptr;
     const MachineFrameInfo *MFI = nullptr;
     MachineRegisterInfo *MRI = nullptr;
+    MachineDomTreeUpdater *MDTU = nullptr;
     TargetSchedModel SchedModel;
     bool PreRegAlloc = false;
     bool HasProfileData = false;
@@ -378,6 +380,10 @@ bool MachineLICMBase::runOnMachineFunction(MachineFunction &MF) {
   DT = &getAnalysis<MachineDominatorTreeWrapperPass>().getDomTree();
   AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
 
+  MachineDomTreeUpdater Updater(DT,
+                                MachineDomTreeUpdater::UpdateStrategy::Lazy);
+  MDTU = &Updater;
+
   if (HoistConstLoads)
     InitializeLoadsHoistableLoops();
 
@@ -1704,7 +1710,8 @@ MachineLICMBase::getCurPreheader(MachineLoop *CurLoop,
         return nullptr;
       }
 
-      CurPreheader = Pred->SplitCriticalEdge(CurLoop->getHeader(), *this);
+      CurPreheader =
+          Pred->SplitCriticalEdge(CurLoop->getHeader(), *this, nullptr, MDTU);
       if (!CurPreheader) {
         CurPreheader = reinterpret_cast<MachineBasicBlock *>(-1);
         return nullptr;
diff --git a/llvm/lib/CodeGen/MachineLoopInfo.cpp b/llvm/lib/CodeGen/MachineLoopInfo.cpp
index a03c008e6045a..ea3f4fbaff479 100644
--- a/llvm/lib/CodeGen/MachineLoopInfo.cpp
+++ b/llvm/lib/CodeGen/MachineLoopInfo.cpp
@@ -77,7 +77,7 @@ bool MachineLoopInfo::invalidate(
 
 void MachineLoopInfo::calculate(MachineDominatorTree &MDT) {
   releaseMemory();
-  analyze(MDT.getBase());
+  analyze(MDT);
 }
 
 void MachineLoopInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index bbc5ab13a0cd3..a621091e62fee 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -30,6 +30,7 @@
 #include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
 #include "llvm/CodeGen/MachineBranchProbabilityInfo.h"
 #include "llvm/CodeGen/MachineCycleAnalysis.h"
+#include "llvm/CodeGen/MachineDomTreeUpdater.h"
 #include "llvm/CodeGen/MachineDominators.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
@@ -728,6 +729,8 @@ boo...
[truncated]

@nikic
Copy link
Contributor

nikic commented Jul 11, 2024

There are a lot of test failures in pre-commit CI.

@paperchalice
Copy link
Contributor Author

Use strategy eager in MachineLICM, because dominator tree is used immediately after updating. Flush updater after all critical edges are split in MachineSink.
If the performance is still unacceptable, I will make another PR to wrap machine dominator tree, the name getBase() doesn't make sense...

@paperchalice
Copy link
Contributor Author

Ping @nikic
How to test the performance? I think I'm not a contributor who regularly does compile-time sensitive work. Thanks.

@nikic
Copy link
Contributor

nikic commented Jul 23, 2024

New results: http://llvm-compile-time-tracker.com/compare.php?from=7d0a5848f2c72854d7d5a795fcc265b8ba787782&to=5217cd0fbdaa35fee4fddf69eaa89f5a381cd37c&stat=instructions:u

I didn't look too closely into this, but note that one of the ways to use DTU is to make all the DT accesses go through it, i.e. DTU->getDomTree(). This should be very close to what the code previously did.

Whether this makes a difference depends on whether the overhead here is due to still too many unbatched DT updates, or because DTU is just less efficient than the previous code for these updates.

@paperchalice
Copy link
Contributor Author

New results: http://llvm-compile-time-tracker.com/compare.php?from=7d0a5848f2c72854d7d5a795fcc265b8ba787782&to=5217cd0fbdaa35fee4fddf69eaa89f5a381cd37c&stat=instructions:u

It still has significant performance impact.

I didn't look too closely into this, but note that one of the ways to use DTU is to make all the DT accesses go through it, i.e. DTU->getDomTree(). This should be very close to what the code previously did.

Whether this makes a difference depends on whether the overhead here is due to still too many unbatched DT updates, or because DTU is just less efficient than the previous code for these updates.

Try to use lazy strategy in MachineLICM.
The initial code is optimized for critical edges, so there is a performance penalty when using generic update method.

@nikic
Copy link
Contributor

nikic commented Jul 25, 2024

I've added your fork to http://llvm-compile-time-tracker.com/, in case you want to continue experimenting with different implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants