Skip to content

Reapply "[DomTreeUpdater] Move critical edge splitting code to updater" #119547

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

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

paperchalice
Copy link
Contributor

@paperchalice paperchalice commented Dec 11, 2024

This relands commit #115111.
Use traditional way to update post dominator tree, i.e. break critical edge splitting into insert, insert, delete sequence.
When splitting critical edges, the post dominator tree may change its root node, and setNewRoot only works in normal dominator tree...
See

DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
assert(getNode(BB) == nullptr && "Block already in dominator tree!");
assert(!this->isPostDominator() &&
"Cannot change root of post-dominator tree");

This reverts commit a28d49b7b54b85b4b2e344a0d4ebc9d511562fee.
@llvmbot
Copy link
Member

llvmbot commented Dec 11, 2024

@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-backend-hexagon

Author: None (paperchalice)

Changes

This reverts commit a28d49b7b54b85b4b2e344a0d4ebc9d511562fee.
When splitting critical edges, the post dominator tree may change its root node, and setNewRoot only works in normal dominator tree...

DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
assert(getNode(BB) == nullptr && "Block already in dominator tree!");
assert(!this->isPostDominator() &&
"Cannot change root of post-dominator tree");


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

30 Files Affected:

  • (modified) llvm/include/llvm/Analysis/DomTreeUpdater.h (+10-3)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdater.h (+36-7)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h (+170-57)
  • (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+16-8)
  • (modified) llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h (+7)
  • (modified) llvm/include/llvm/CodeGen/MachineDominators.h (+2-167)
  • (modified) llvm/include/llvm/CodeGen/MachineSSAContext.h (-6)
  • (modified) llvm/lib/Analysis/DomTreeUpdater.cpp (+7)
  • (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 (+5-3)
  • (modified) llvm/lib/CodeGen/MachineDomTreeUpdater.cpp (+7)
  • (modified) llvm/lib/CodeGen/MachineDominanceFrontier.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/MachineDominators.cpp (-74)
  • (modified) llvm/lib/CodeGen/MachineLICM.cpp (+10-7)
  • (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 (+18-9)
  • (modified) llvm/lib/CodeGen/XRayInstrumentation.cpp (+2-2)
  • (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/Analysis/DomTreeUpdaterTest.cpp (+43)
  • (modified) llvm/unittests/Target/WebAssembly/WebAssemblyExceptionInfoTest.cpp (+4-4)
diff --git a/llvm/include/llvm/Analysis/DomTreeUpdater.h b/llvm/include/llvm/Analysis/DomTreeUpdater.h
index c120a6cc6ce5abe..0386262ba2b653e 100644
--- a/llvm/include/llvm/Analysis/DomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/DomTreeUpdater.h
@@ -81,6 +81,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:
@@ -109,9 +112,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,
@@ -120,6 +120,13 @@ extern template class GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
 extern template void
 GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
                       PostDominatorTree>::recalculate(Function &F);
+
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/true>();
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/false>();
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_DOMTREEUPDATER_H
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
index ca4ce68b85cbcfc..4a03f548823ee47 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -30,6 +30,7 @@ class GenericDomTreeUpdater {
 public:
   enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };
   using BasicBlockT = typename DomTreeT::NodeType;
+  using UpdateT = typename DomTreeT::UpdateType;
 
   explicit GenericDomTreeUpdater(UpdateStrategy Strategy_)
       : Strategy(Strategy_) {}
@@ -146,7 +147,12 @@ 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.
-  void applyUpdates(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdates(ArrayRef<UpdateT> Updates);
+
+  /// Apply updates that the critical edge (FromBB, ToBB) has been
+  /// split with NewBB.
+  void splitCriticalEdge(BasicBlockT *FromBB, BasicBlockT *ToBB,
+                         BasicBlockT *NewBB);
 
   /// Submit updates to all available trees. It will also
   /// 1. discard duplicated updates,
@@ -169,7 +175,7 @@ class GenericDomTreeUpdater {
   /// 3. It is only legal to submit updates to an edge in the order CFG changes
   /// are made. The order you submit updates on different edges is not
   /// restricted.
-  void applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdatesPermissive(ArrayRef<UpdateT> Updates);
 
   ///@}
 
@@ -205,7 +211,25 @@ class GenericDomTreeUpdater {
   LLVM_DUMP_METHOD void dump() const;
 
 protected:
-  SmallVector<typename DomTreeT::UpdateType, 16> PendUpdates;
+  /// 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;
+  };
+
+  struct DomTreeUpdate {
+    bool IsCriticalEdgeSplit = false;
+    union {
+      UpdateT Update;
+      CriticalEdge EdgeSplit;
+    };
+    DomTreeUpdate(UpdateT Update) : Update(Update) {}
+    DomTreeUpdate(CriticalEdge E) : IsCriticalEdgeSplit(true), EdgeSplit(E) {}
+  };
+
+  SmallVector<DomTreeUpdate, 16> PendUpdates;
   size_t PendDTUpdateIndex = 0;
   size_t PendPDTUpdateIndex = 0;
   DomTreeT *DT = nullptr;
@@ -216,21 +240,21 @@ class GenericDomTreeUpdater {
   bool IsRecalculatingPostDomTree = false;
 
   /// Returns true if the update is self dominance.
-  bool isSelfDominance(typename DomTreeT::UpdateType Update) const {
+  bool isSelfDominance(UpdateT Update) const {
     // Won't affect DomTree and PostDomTree.
     return Update.getFrom() == Update.getTo();
   }
 
   /// Helper function to apply all pending DomTree updates.
-  void applyDomTreeUpdates();
+  void applyDomTreeUpdates() { applyUpdatesImpl<true>(); }
 
   /// Helper function to apply all pending PostDomTree updates.
-  void applyPostDomTreeUpdates();
+  void applyPostDomTreeUpdates() { applyUpdatesImpl<false>(); }
 
   /// Returns true if the update appears in the LLVM IR.
   /// It is used to check whether an update is valid in
   /// insertEdge/deleteEdge or is unnecessary in the batch update.
-  bool isUpdateValid(typename DomTreeT::UpdateType Update) const;
+  bool isUpdateValid(UpdateT Update) const;
 
   /// Erase Basic Block node before it is unlinked from Function
   /// in the DomTree and PostDomTree.
@@ -243,6 +267,11 @@ class GenericDomTreeUpdater {
   /// Drop all updates applied by all available trees and delete BasicBlocks if
   /// all available trees are up-to-date.
   void dropOutOfDateUpdates();
+
+private:
+  void splitDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  void splitPDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  template <bool IsForward> void applyUpdatesImpl();
 };
 
 } // namespace llvm
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
index b79eaef57710fa8..896b68c5021b3b0 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
@@ -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"
@@ -56,7 +57,7 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::recalculate(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
-    ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
@@ -77,12 +78,12 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
-    applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    applyUpdatesPermissive(ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
   SmallSet<std::pair<BasicBlockT *, BasicBlockT *>, 8> Seen;
-  SmallVector<typename DomTreeT::UpdateType, 8> DeduplicatedUpdates;
+  SmallVector<UpdateT, 8> DeduplicatedUpdates;
   for (const auto &U : Updates) {
     auto Edge = std::make_pair(U.getFrom(), U.getTo());
     // Because it is illegal to submit updates that have already been applied
@@ -129,6 +130,24 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
     PDT->applyUpdates(DeduplicatedUpdates);
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::splitCriticalEdge(
+    BasicBlockT *FromBB, BasicBlockT *ToBB, BasicBlockT *NewBB) {
+  if (!DT && !PDT)
+    return;
+
+  CriticalEdge E = {FromBB, ToBB, NewBB};
+  if (Strategy == UpdateStrategy::Lazy) {
+    PendUpdates.push_back(E);
+    return;
+  }
+
+  if (DT)
+    splitDTCriticalEdges(E);
+  if (PDT)
+    splitPDTCriticalEdges(E);
+}
+
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 DomTreeT &
 GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::getDomTree() {
@@ -171,39 +190,40 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     OS << "Lazy\n";
   int Index = 0;
 
+  auto printBlockInfo = [&](BasicBlockT *BB, StringRef Ending) {
+    if (BB) {
+      auto S = BB->getName();
+      if (!BB->hasName())
+        S = "(no name)";
+      OS << S << "(" << BB << ")" << Ending;
+    } else {
+      OS << "(badref)" << Ending;
+    }
+  };
+
   auto printUpdates =
-      [&](typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              begin,
-          typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              end) {
+      [&](typename ArrayRef<DomTreeUpdate>::const_iterator begin,
+          typename ArrayRef<DomTreeUpdate>::const_iterator end) {
         if (begin == end)
           OS << "  None\n";
         Index = 0;
         for (auto It = begin, ItEnd = end; It != ItEnd; ++It) {
-          auto U = *It;
-          OS << "  " << Index << " : ";
-          ++Index;
-          if (U.getKind() == DomTreeT::Insert)
-            OS << "Insert, ";
-          else
-            OS << "Delete, ";
-          BasicBlockT *From = U.getFrom();
-          if (From) {
-            auto S = From->getName();
-            if (!From->hasName())
-              S = "(no name)";
-            OS << S << "(" << From << "), ";
-          } else {
-            OS << "(badref), ";
-          }
-          BasicBlockT *To = U.getTo();
-          if (To) {
-            auto S = To->getName();
-            if (!To->hasName())
-              S = "(no_name)";
-            OS << S << "(" << To << ")\n";
+          if (!It->IsCriticalEdgeSplit) {
+            auto U = It->Update;
+            OS << "  " << Index << " : ";
+            ++Index;
+            if (U.getKind() == DomTreeT::Insert)
+              OS << "Insert, ";
+            else
+              OS << "Delete, ";
+            printBlockInfo(U.getFrom(), ", ");
+            printBlockInfo(U.getTo(), "\n");
           } else {
-            OS << "(badref)\n";
+            const auto &Edge = It->EdgeSplit;
+            OS << "  " << Index++ << " : Split critical edge, ";
+            printBlockInfo(Edge.FromBB, ", ");
+            printBlockInfo(Edge.ToBB, ", ");
+            printBlockInfo(Edge.NewBB, "\n");
           }
         }
       };
@@ -236,50 +256,53 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     if (BB->hasName())
       OS << BB->getName() << "(";
     else
-      OS << "(no_name)(";
+      OS << "(no name)(";
     OS << BB << ")\n";
   }
 #endif
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+template <bool IsForward>
 void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyDomTreeUpdates() {
+                           PostDomTreeT>::applyUpdatesImpl() {
+  auto *DomTree = [&]() {
+    if constexpr (IsForward)
+      return DT;
+    else
+      return PDT;
+  }();
   // No pending DomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !DT)
+  if (Strategy != UpdateStrategy::Lazy || !DomTree)
     return;
+  size_t &PendUpdateIndex = IsForward ? PendDTUpdateIndex : PendPDTUpdateIndex;
 
-  // Only apply updates not are applied by DomTree.
-  if (hasPendingDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendDTUpdateIndex;
+  // Only apply updates not are applied by (Post)DomTree.
+  while (IsForward ? hasPendingDomTreeUpdates()
+                   : hasPendingPostDomTreeUpdates()) {
+    auto I = PendUpdates.begin() + PendUpdateIndex;
     const auto E = PendUpdates.end();
     assert(I < E && "Iterator range invalid; there should be DomTree updates.");
-    DT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendDTUpdateIndex = PendUpdates.size();
-  }
-}
-
-template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
-void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyPostDomTreeUpdates() {
-  // No pending PostDomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !PDT)
-    return;
-
-  // Only apply updates not are applied by PostDomTree.
-  if (hasPendingPostDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendPDTUpdateIndex;
-    const auto E = PendUpdates.end();
-    assert(I < E &&
-           "Iterator range invalid; there should be PostDomTree updates.");
-    PDT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendPDTUpdateIndex = PendUpdates.size();
+    if (!I->IsCriticalEdgeSplit) {
+      SmallVector<UpdateT, 32> NormalUpdates;
+      for (; I != E && !I->IsCriticalEdgeSplit; ++I)
+        NormalUpdates.push_back(I->Update);
+      DomTree->applyUpdates(NormalUpdates);
+      PendUpdateIndex += NormalUpdates.size();
+    } else {
+      SmallVector<CriticalEdge> CriticalEdges;
+      for (; I != E && I->IsCriticalEdgeSplit; ++I)
+        CriticalEdges.push_back(I->EdgeSplit);
+      IsForward ? splitDTCriticalEdges(CriticalEdges)
+                : splitPDTCriticalEdges(CriticalEdges);
+      PendUpdateIndex += CriticalEdges.size();
+    }
   }
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 bool GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::isUpdateValid(
-    typename DomTreeT::UpdateType Update) const {
+    UpdateT Update) const {
   const auto *From = Update.getFrom();
   const auto *To = Update.getTo();
   const auto Kind = Update.getKind();
@@ -347,6 +370,96 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT,
   PendPDTUpdateIndex -= dropIndex;
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!DT || Edges.empty())
+    return;
+
+  // 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 Edges.
+  // I.e., forall elt in Edges, it exists BB in NewBBs
+  // such as BB == elt.NewBB.
+  SmallSet<BasicBlockT *, 32> NewBBs;
+  for (auto &Edge : Edges)
+    NewBBs.insert(Edge.NewBB);
+  // For each element in Edges, 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 Edges is
+  // the ith element of IsNewIDom.
+  SmallBitVector IsNewIDom(Edges.size(), true);
+
+  // Collect all the dominance properties info, before invalidating
+  // the underlying DT.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // Update dominator information.
+    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.contains(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;
+      }
+    }
+  }
+
+  // Now, update DT with the collected dominance properties info.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // 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);
+  }
+}
+
+// Post dominator tree is different, the new basic block in critical edge
+// may become the new root.
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitPDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!PDT || Edges.empty())
+    return;
+
+  std::vector<UpdateT> Updates;
+  for (const auto &Edge : Edges) {
+    Updates.push_back({PostDomTreeT::Insert, Edge.FromBB, Edge.NewBB});
+    Updates.push_back({PostDomTreeT::Insert, Edge.NewBB, Edge.ToBB});
+    if (!llvm::is_contained(successors(Edge.FromBB), Edge.ToBB))
+      Updates.push_back({PostDomTreeT::Delete, Edge.FromBB, Edge.ToBB});
+  }
+  PDT->applyUpdates(Updates);
+}
+
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_GENERICDOMTREEUPDATERIMPL_H
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 6cf151c951b19f4..7fe33c3913f2dd4 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -32,6 +32,7 @@
 namespace llvm {
 
 class BasicBlock;
+class MachineDomTreeUpdater;
 class MachineFunction;
 class MCSymbol;
 class ModuleSlotTracker;
@@ -972,22 +973,23 @@ 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);
   }
 
   // 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);
 
   /// Check if the edge between this block and the given successor \p
   /// Succ, can be split. If this returns true a subsequent call to
@@ -1375,6 +1377,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
diff --git a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
index 9e3971f0b9fcecc..fcdc0becf31c12b 100644
--- a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
+++ b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
@@ -69,5 +69,12 @@ extern template void
 GenericDomTreeUpdater<MachineDomTreeUpdater, MachineDominatorTree,
                       MachinePostDominatorTree>::recalculate(MachineFunction
                                                                  &MF);
+
+extern template void GenericDomTreeUpdater<
+    Ma...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Dec 11, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: None (paperchalice)

Changes

This reverts commit a28d49b7b54b85b4b2e344a0d4ebc9d511562fee.
When splitting critical edges, the post dominator tree may change its root node, and setNewRoot only works in normal dominator tree...

DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
assert(getNode(BB) == nullptr && "Block already in dominator tree!");
assert(!this->isPostDominator() &&
"Cannot change root of post-dominator tree");


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

30 Files Affected:

  • (modified) llvm/include/llvm/Analysis/DomTreeUpdater.h (+10-3)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdater.h (+36-7)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h (+170-57)
  • (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+16-8)
  • (modified) llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h (+7)
  • (modified) llvm/include/llvm/CodeGen/MachineDominators.h (+2-167)
  • (modified) llvm/include/llvm/CodeGen/MachineSSAContext.h (-6)
  • (modified) llvm/lib/Analysis/DomTreeUpdater.cpp (+7)
  • (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 (+5-3)
  • (modified) llvm/lib/CodeGen/MachineDomTreeUpdater.cpp (+7)
  • (modified) llvm/lib/CodeGen/MachineDominanceFrontier.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/MachineDominators.cpp (-74)
  • (modified) llvm/lib/CodeGen/MachineLICM.cpp (+10-7)
  • (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 (+18-9)
  • (modified) llvm/lib/CodeGen/XRayInstrumentation.cpp (+2-2)
  • (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/Analysis/DomTreeUpdaterTest.cpp (+43)
  • (modified) llvm/unittests/Target/WebAssembly/WebAssemblyExceptionInfoTest.cpp (+4-4)
diff --git a/llvm/include/llvm/Analysis/DomTreeUpdater.h b/llvm/include/llvm/Analysis/DomTreeUpdater.h
index c120a6cc6ce5abe..0386262ba2b653e 100644
--- a/llvm/include/llvm/Analysis/DomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/DomTreeUpdater.h
@@ -81,6 +81,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:
@@ -109,9 +112,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,
@@ -120,6 +120,13 @@ extern template class GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
 extern template void
 GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
                       PostDominatorTree>::recalculate(Function &F);
+
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/true>();
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/false>();
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_DOMTREEUPDATER_H
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
index ca4ce68b85cbcfc..4a03f548823ee47 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -30,6 +30,7 @@ class GenericDomTreeUpdater {
 public:
   enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };
   using BasicBlockT = typename DomTreeT::NodeType;
+  using UpdateT = typename DomTreeT::UpdateType;
 
   explicit GenericDomTreeUpdater(UpdateStrategy Strategy_)
       : Strategy(Strategy_) {}
@@ -146,7 +147,12 @@ 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.
-  void applyUpdates(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdates(ArrayRef<UpdateT> Updates);
+
+  /// Apply updates that the critical edge (FromBB, ToBB) has been
+  /// split with NewBB.
+  void splitCriticalEdge(BasicBlockT *FromBB, BasicBlockT *ToBB,
+                         BasicBlockT *NewBB);
 
   /// Submit updates to all available trees. It will also
   /// 1. discard duplicated updates,
@@ -169,7 +175,7 @@ class GenericDomTreeUpdater {
   /// 3. It is only legal to submit updates to an edge in the order CFG changes
   /// are made. The order you submit updates on different edges is not
   /// restricted.
-  void applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdatesPermissive(ArrayRef<UpdateT> Updates);
 
   ///@}
 
@@ -205,7 +211,25 @@ class GenericDomTreeUpdater {
   LLVM_DUMP_METHOD void dump() const;
 
 protected:
-  SmallVector<typename DomTreeT::UpdateType, 16> PendUpdates;
+  /// 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;
+  };
+
+  struct DomTreeUpdate {
+    bool IsCriticalEdgeSplit = false;
+    union {
+      UpdateT Update;
+      CriticalEdge EdgeSplit;
+    };
+    DomTreeUpdate(UpdateT Update) : Update(Update) {}
+    DomTreeUpdate(CriticalEdge E) : IsCriticalEdgeSplit(true), EdgeSplit(E) {}
+  };
+
+  SmallVector<DomTreeUpdate, 16> PendUpdates;
   size_t PendDTUpdateIndex = 0;
   size_t PendPDTUpdateIndex = 0;
   DomTreeT *DT = nullptr;
@@ -216,21 +240,21 @@ class GenericDomTreeUpdater {
   bool IsRecalculatingPostDomTree = false;
 
   /// Returns true if the update is self dominance.
-  bool isSelfDominance(typename DomTreeT::UpdateType Update) const {
+  bool isSelfDominance(UpdateT Update) const {
     // Won't affect DomTree and PostDomTree.
     return Update.getFrom() == Update.getTo();
   }
 
   /// Helper function to apply all pending DomTree updates.
-  void applyDomTreeUpdates();
+  void applyDomTreeUpdates() { applyUpdatesImpl<true>(); }
 
   /// Helper function to apply all pending PostDomTree updates.
-  void applyPostDomTreeUpdates();
+  void applyPostDomTreeUpdates() { applyUpdatesImpl<false>(); }
 
   /// Returns true if the update appears in the LLVM IR.
   /// It is used to check whether an update is valid in
   /// insertEdge/deleteEdge or is unnecessary in the batch update.
-  bool isUpdateValid(typename DomTreeT::UpdateType Update) const;
+  bool isUpdateValid(UpdateT Update) const;
 
   /// Erase Basic Block node before it is unlinked from Function
   /// in the DomTree and PostDomTree.
@@ -243,6 +267,11 @@ class GenericDomTreeUpdater {
   /// Drop all updates applied by all available trees and delete BasicBlocks if
   /// all available trees are up-to-date.
   void dropOutOfDateUpdates();
+
+private:
+  void splitDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  void splitPDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  template <bool IsForward> void applyUpdatesImpl();
 };
 
 } // namespace llvm
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
index b79eaef57710fa8..896b68c5021b3b0 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
@@ -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"
@@ -56,7 +57,7 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::recalculate(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
-    ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
@@ -77,12 +78,12 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
-    applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    applyUpdatesPermissive(ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
   SmallSet<std::pair<BasicBlockT *, BasicBlockT *>, 8> Seen;
-  SmallVector<typename DomTreeT::UpdateType, 8> DeduplicatedUpdates;
+  SmallVector<UpdateT, 8> DeduplicatedUpdates;
   for (const auto &U : Updates) {
     auto Edge = std::make_pair(U.getFrom(), U.getTo());
     // Because it is illegal to submit updates that have already been applied
@@ -129,6 +130,24 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
     PDT->applyUpdates(DeduplicatedUpdates);
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::splitCriticalEdge(
+    BasicBlockT *FromBB, BasicBlockT *ToBB, BasicBlockT *NewBB) {
+  if (!DT && !PDT)
+    return;
+
+  CriticalEdge E = {FromBB, ToBB, NewBB};
+  if (Strategy == UpdateStrategy::Lazy) {
+    PendUpdates.push_back(E);
+    return;
+  }
+
+  if (DT)
+    splitDTCriticalEdges(E);
+  if (PDT)
+    splitPDTCriticalEdges(E);
+}
+
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 DomTreeT &
 GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::getDomTree() {
@@ -171,39 +190,40 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     OS << "Lazy\n";
   int Index = 0;
 
+  auto printBlockInfo = [&](BasicBlockT *BB, StringRef Ending) {
+    if (BB) {
+      auto S = BB->getName();
+      if (!BB->hasName())
+        S = "(no name)";
+      OS << S << "(" << BB << ")" << Ending;
+    } else {
+      OS << "(badref)" << Ending;
+    }
+  };
+
   auto printUpdates =
-      [&](typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              begin,
-          typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              end) {
+      [&](typename ArrayRef<DomTreeUpdate>::const_iterator begin,
+          typename ArrayRef<DomTreeUpdate>::const_iterator end) {
         if (begin == end)
           OS << "  None\n";
         Index = 0;
         for (auto It = begin, ItEnd = end; It != ItEnd; ++It) {
-          auto U = *It;
-          OS << "  " << Index << " : ";
-          ++Index;
-          if (U.getKind() == DomTreeT::Insert)
-            OS << "Insert, ";
-          else
-            OS << "Delete, ";
-          BasicBlockT *From = U.getFrom();
-          if (From) {
-            auto S = From->getName();
-            if (!From->hasName())
-              S = "(no name)";
-            OS << S << "(" << From << "), ";
-          } else {
-            OS << "(badref), ";
-          }
-          BasicBlockT *To = U.getTo();
-          if (To) {
-            auto S = To->getName();
-            if (!To->hasName())
-              S = "(no_name)";
-            OS << S << "(" << To << ")\n";
+          if (!It->IsCriticalEdgeSplit) {
+            auto U = It->Update;
+            OS << "  " << Index << " : ";
+            ++Index;
+            if (U.getKind() == DomTreeT::Insert)
+              OS << "Insert, ";
+            else
+              OS << "Delete, ";
+            printBlockInfo(U.getFrom(), ", ");
+            printBlockInfo(U.getTo(), "\n");
           } else {
-            OS << "(badref)\n";
+            const auto &Edge = It->EdgeSplit;
+            OS << "  " << Index++ << " : Split critical edge, ";
+            printBlockInfo(Edge.FromBB, ", ");
+            printBlockInfo(Edge.ToBB, ", ");
+            printBlockInfo(Edge.NewBB, "\n");
           }
         }
       };
@@ -236,50 +256,53 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     if (BB->hasName())
       OS << BB->getName() << "(";
     else
-      OS << "(no_name)(";
+      OS << "(no name)(";
     OS << BB << ")\n";
   }
 #endif
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+template <bool IsForward>
 void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyDomTreeUpdates() {
+                           PostDomTreeT>::applyUpdatesImpl() {
+  auto *DomTree = [&]() {
+    if constexpr (IsForward)
+      return DT;
+    else
+      return PDT;
+  }();
   // No pending DomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !DT)
+  if (Strategy != UpdateStrategy::Lazy || !DomTree)
     return;
+  size_t &PendUpdateIndex = IsForward ? PendDTUpdateIndex : PendPDTUpdateIndex;
 
-  // Only apply updates not are applied by DomTree.
-  if (hasPendingDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendDTUpdateIndex;
+  // Only apply updates not are applied by (Post)DomTree.
+  while (IsForward ? hasPendingDomTreeUpdates()
+                   : hasPendingPostDomTreeUpdates()) {
+    auto I = PendUpdates.begin() + PendUpdateIndex;
     const auto E = PendUpdates.end();
     assert(I < E && "Iterator range invalid; there should be DomTree updates.");
-    DT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendDTUpdateIndex = PendUpdates.size();
-  }
-}
-
-template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
-void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyPostDomTreeUpdates() {
-  // No pending PostDomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !PDT)
-    return;
-
-  // Only apply updates not are applied by PostDomTree.
-  if (hasPendingPostDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendPDTUpdateIndex;
-    const auto E = PendUpdates.end();
-    assert(I < E &&
-           "Iterator range invalid; there should be PostDomTree updates.");
-    PDT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendPDTUpdateIndex = PendUpdates.size();
+    if (!I->IsCriticalEdgeSplit) {
+      SmallVector<UpdateT, 32> NormalUpdates;
+      for (; I != E && !I->IsCriticalEdgeSplit; ++I)
+        NormalUpdates.push_back(I->Update);
+      DomTree->applyUpdates(NormalUpdates);
+      PendUpdateIndex += NormalUpdates.size();
+    } else {
+      SmallVector<CriticalEdge> CriticalEdges;
+      for (; I != E && I->IsCriticalEdgeSplit; ++I)
+        CriticalEdges.push_back(I->EdgeSplit);
+      IsForward ? splitDTCriticalEdges(CriticalEdges)
+                : splitPDTCriticalEdges(CriticalEdges);
+      PendUpdateIndex += CriticalEdges.size();
+    }
   }
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 bool GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::isUpdateValid(
-    typename DomTreeT::UpdateType Update) const {
+    UpdateT Update) const {
   const auto *From = Update.getFrom();
   const auto *To = Update.getTo();
   const auto Kind = Update.getKind();
@@ -347,6 +370,96 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT,
   PendPDTUpdateIndex -= dropIndex;
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!DT || Edges.empty())
+    return;
+
+  // 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 Edges.
+  // I.e., forall elt in Edges, it exists BB in NewBBs
+  // such as BB == elt.NewBB.
+  SmallSet<BasicBlockT *, 32> NewBBs;
+  for (auto &Edge : Edges)
+    NewBBs.insert(Edge.NewBB);
+  // For each element in Edges, 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 Edges is
+  // the ith element of IsNewIDom.
+  SmallBitVector IsNewIDom(Edges.size(), true);
+
+  // Collect all the dominance properties info, before invalidating
+  // the underlying DT.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // Update dominator information.
+    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.contains(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;
+      }
+    }
+  }
+
+  // Now, update DT with the collected dominance properties info.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // 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);
+  }
+}
+
+// Post dominator tree is different, the new basic block in critical edge
+// may become the new root.
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitPDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!PDT || Edges.empty())
+    return;
+
+  std::vector<UpdateT> Updates;
+  for (const auto &Edge : Edges) {
+    Updates.push_back({PostDomTreeT::Insert, Edge.FromBB, Edge.NewBB});
+    Updates.push_back({PostDomTreeT::Insert, Edge.NewBB, Edge.ToBB});
+    if (!llvm::is_contained(successors(Edge.FromBB), Edge.ToBB))
+      Updates.push_back({PostDomTreeT::Delete, Edge.FromBB, Edge.ToBB});
+  }
+  PDT->applyUpdates(Updates);
+}
+
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_GENERICDOMTREEUPDATERIMPL_H
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 6cf151c951b19f4..7fe33c3913f2dd4 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -32,6 +32,7 @@
 namespace llvm {
 
 class BasicBlock;
+class MachineDomTreeUpdater;
 class MachineFunction;
 class MCSymbol;
 class ModuleSlotTracker;
@@ -972,22 +973,23 @@ 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);
   }
 
   // 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);
 
   /// Check if the edge between this block and the given successor \p
   /// Succ, can be split. If this returns true a subsequent call to
@@ -1375,6 +1377,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
diff --git a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
index 9e3971f0b9fcecc..fcdc0becf31c12b 100644
--- a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
+++ b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
@@ -69,5 +69,12 @@ extern template void
 GenericDomTreeUpdater<MachineDomTreeUpdater, MachineDominatorTree,
                       MachinePostDominatorTree>::recalculate(MachineFunction
                                                                  &MF);
+
+extern template void GenericDomTreeUpdater<
+    Ma...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Dec 11, 2024

@llvm/pr-subscribers-llvm-analysis

Author: None (paperchalice)

Changes

This reverts commit a28d49b7b54b85b4b2e344a0d4ebc9d511562fee.
When splitting critical edges, the post dominator tree may change its root node, and setNewRoot only works in normal dominator tree...

DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
assert(getNode(BB) == nullptr && "Block already in dominator tree!");
assert(!this->isPostDominator() &&
"Cannot change root of post-dominator tree");


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

30 Files Affected:

  • (modified) llvm/include/llvm/Analysis/DomTreeUpdater.h (+10-3)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdater.h (+36-7)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h (+170-57)
  • (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+16-8)
  • (modified) llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h (+7)
  • (modified) llvm/include/llvm/CodeGen/MachineDominators.h (+2-167)
  • (modified) llvm/include/llvm/CodeGen/MachineSSAContext.h (-6)
  • (modified) llvm/lib/Analysis/DomTreeUpdater.cpp (+7)
  • (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 (+5-3)
  • (modified) llvm/lib/CodeGen/MachineDomTreeUpdater.cpp (+7)
  • (modified) llvm/lib/CodeGen/MachineDominanceFrontier.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/MachineDominators.cpp (-74)
  • (modified) llvm/lib/CodeGen/MachineLICM.cpp (+10-7)
  • (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 (+18-9)
  • (modified) llvm/lib/CodeGen/XRayInstrumentation.cpp (+2-2)
  • (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/Analysis/DomTreeUpdaterTest.cpp (+43)
  • (modified) llvm/unittests/Target/WebAssembly/WebAssemblyExceptionInfoTest.cpp (+4-4)
diff --git a/llvm/include/llvm/Analysis/DomTreeUpdater.h b/llvm/include/llvm/Analysis/DomTreeUpdater.h
index c120a6cc6ce5abe..0386262ba2b653e 100644
--- a/llvm/include/llvm/Analysis/DomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/DomTreeUpdater.h
@@ -81,6 +81,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:
@@ -109,9 +112,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,
@@ -120,6 +120,13 @@ extern template class GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
 extern template void
 GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
                       PostDominatorTree>::recalculate(Function &F);
+
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/true>();
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/false>();
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_DOMTREEUPDATER_H
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
index ca4ce68b85cbcfc..4a03f548823ee47 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -30,6 +30,7 @@ class GenericDomTreeUpdater {
 public:
   enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };
   using BasicBlockT = typename DomTreeT::NodeType;
+  using UpdateT = typename DomTreeT::UpdateType;
 
   explicit GenericDomTreeUpdater(UpdateStrategy Strategy_)
       : Strategy(Strategy_) {}
@@ -146,7 +147,12 @@ 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.
-  void applyUpdates(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdates(ArrayRef<UpdateT> Updates);
+
+  /// Apply updates that the critical edge (FromBB, ToBB) has been
+  /// split with NewBB.
+  void splitCriticalEdge(BasicBlockT *FromBB, BasicBlockT *ToBB,
+                         BasicBlockT *NewBB);
 
   /// Submit updates to all available trees. It will also
   /// 1. discard duplicated updates,
@@ -169,7 +175,7 @@ class GenericDomTreeUpdater {
   /// 3. It is only legal to submit updates to an edge in the order CFG changes
   /// are made. The order you submit updates on different edges is not
   /// restricted.
-  void applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdatesPermissive(ArrayRef<UpdateT> Updates);
 
   ///@}
 
@@ -205,7 +211,25 @@ class GenericDomTreeUpdater {
   LLVM_DUMP_METHOD void dump() const;
 
 protected:
-  SmallVector<typename DomTreeT::UpdateType, 16> PendUpdates;
+  /// 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;
+  };
+
+  struct DomTreeUpdate {
+    bool IsCriticalEdgeSplit = false;
+    union {
+      UpdateT Update;
+      CriticalEdge EdgeSplit;
+    };
+    DomTreeUpdate(UpdateT Update) : Update(Update) {}
+    DomTreeUpdate(CriticalEdge E) : IsCriticalEdgeSplit(true), EdgeSplit(E) {}
+  };
+
+  SmallVector<DomTreeUpdate, 16> PendUpdates;
   size_t PendDTUpdateIndex = 0;
   size_t PendPDTUpdateIndex = 0;
   DomTreeT *DT = nullptr;
@@ -216,21 +240,21 @@ class GenericDomTreeUpdater {
   bool IsRecalculatingPostDomTree = false;
 
   /// Returns true if the update is self dominance.
-  bool isSelfDominance(typename DomTreeT::UpdateType Update) const {
+  bool isSelfDominance(UpdateT Update) const {
     // Won't affect DomTree and PostDomTree.
     return Update.getFrom() == Update.getTo();
   }
 
   /// Helper function to apply all pending DomTree updates.
-  void applyDomTreeUpdates();
+  void applyDomTreeUpdates() { applyUpdatesImpl<true>(); }
 
   /// Helper function to apply all pending PostDomTree updates.
-  void applyPostDomTreeUpdates();
+  void applyPostDomTreeUpdates() { applyUpdatesImpl<false>(); }
 
   /// Returns true if the update appears in the LLVM IR.
   /// It is used to check whether an update is valid in
   /// insertEdge/deleteEdge or is unnecessary in the batch update.
-  bool isUpdateValid(typename DomTreeT::UpdateType Update) const;
+  bool isUpdateValid(UpdateT Update) const;
 
   /// Erase Basic Block node before it is unlinked from Function
   /// in the DomTree and PostDomTree.
@@ -243,6 +267,11 @@ class GenericDomTreeUpdater {
   /// Drop all updates applied by all available trees and delete BasicBlocks if
   /// all available trees are up-to-date.
   void dropOutOfDateUpdates();
+
+private:
+  void splitDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  void splitPDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  template <bool IsForward> void applyUpdatesImpl();
 };
 
 } // namespace llvm
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
index b79eaef57710fa8..896b68c5021b3b0 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
@@ -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"
@@ -56,7 +57,7 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::recalculate(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
-    ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
@@ -77,12 +78,12 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
-    applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    applyUpdatesPermissive(ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
   SmallSet<std::pair<BasicBlockT *, BasicBlockT *>, 8> Seen;
-  SmallVector<typename DomTreeT::UpdateType, 8> DeduplicatedUpdates;
+  SmallVector<UpdateT, 8> DeduplicatedUpdates;
   for (const auto &U : Updates) {
     auto Edge = std::make_pair(U.getFrom(), U.getTo());
     // Because it is illegal to submit updates that have already been applied
@@ -129,6 +130,24 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
     PDT->applyUpdates(DeduplicatedUpdates);
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::splitCriticalEdge(
+    BasicBlockT *FromBB, BasicBlockT *ToBB, BasicBlockT *NewBB) {
+  if (!DT && !PDT)
+    return;
+
+  CriticalEdge E = {FromBB, ToBB, NewBB};
+  if (Strategy == UpdateStrategy::Lazy) {
+    PendUpdates.push_back(E);
+    return;
+  }
+
+  if (DT)
+    splitDTCriticalEdges(E);
+  if (PDT)
+    splitPDTCriticalEdges(E);
+}
+
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 DomTreeT &
 GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::getDomTree() {
@@ -171,39 +190,40 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     OS << "Lazy\n";
   int Index = 0;
 
+  auto printBlockInfo = [&](BasicBlockT *BB, StringRef Ending) {
+    if (BB) {
+      auto S = BB->getName();
+      if (!BB->hasName())
+        S = "(no name)";
+      OS << S << "(" << BB << ")" << Ending;
+    } else {
+      OS << "(badref)" << Ending;
+    }
+  };
+
   auto printUpdates =
-      [&](typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              begin,
-          typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              end) {
+      [&](typename ArrayRef<DomTreeUpdate>::const_iterator begin,
+          typename ArrayRef<DomTreeUpdate>::const_iterator end) {
         if (begin == end)
           OS << "  None\n";
         Index = 0;
         for (auto It = begin, ItEnd = end; It != ItEnd; ++It) {
-          auto U = *It;
-          OS << "  " << Index << " : ";
-          ++Index;
-          if (U.getKind() == DomTreeT::Insert)
-            OS << "Insert, ";
-          else
-            OS << "Delete, ";
-          BasicBlockT *From = U.getFrom();
-          if (From) {
-            auto S = From->getName();
-            if (!From->hasName())
-              S = "(no name)";
-            OS << S << "(" << From << "), ";
-          } else {
-            OS << "(badref), ";
-          }
-          BasicBlockT *To = U.getTo();
-          if (To) {
-            auto S = To->getName();
-            if (!To->hasName())
-              S = "(no_name)";
-            OS << S << "(" << To << ")\n";
+          if (!It->IsCriticalEdgeSplit) {
+            auto U = It->Update;
+            OS << "  " << Index << " : ";
+            ++Index;
+            if (U.getKind() == DomTreeT::Insert)
+              OS << "Insert, ";
+            else
+              OS << "Delete, ";
+            printBlockInfo(U.getFrom(), ", ");
+            printBlockInfo(U.getTo(), "\n");
           } else {
-            OS << "(badref)\n";
+            const auto &Edge = It->EdgeSplit;
+            OS << "  " << Index++ << " : Split critical edge, ";
+            printBlockInfo(Edge.FromBB, ", ");
+            printBlockInfo(Edge.ToBB, ", ");
+            printBlockInfo(Edge.NewBB, "\n");
           }
         }
       };
@@ -236,50 +256,53 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     if (BB->hasName())
       OS << BB->getName() << "(";
     else
-      OS << "(no_name)(";
+      OS << "(no name)(";
     OS << BB << ")\n";
   }
 #endif
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+template <bool IsForward>
 void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyDomTreeUpdates() {
+                           PostDomTreeT>::applyUpdatesImpl() {
+  auto *DomTree = [&]() {
+    if constexpr (IsForward)
+      return DT;
+    else
+      return PDT;
+  }();
   // No pending DomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !DT)
+  if (Strategy != UpdateStrategy::Lazy || !DomTree)
     return;
+  size_t &PendUpdateIndex = IsForward ? PendDTUpdateIndex : PendPDTUpdateIndex;
 
-  // Only apply updates not are applied by DomTree.
-  if (hasPendingDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendDTUpdateIndex;
+  // Only apply updates not are applied by (Post)DomTree.
+  while (IsForward ? hasPendingDomTreeUpdates()
+                   : hasPendingPostDomTreeUpdates()) {
+    auto I = PendUpdates.begin() + PendUpdateIndex;
     const auto E = PendUpdates.end();
     assert(I < E && "Iterator range invalid; there should be DomTree updates.");
-    DT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendDTUpdateIndex = PendUpdates.size();
-  }
-}
-
-template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
-void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyPostDomTreeUpdates() {
-  // No pending PostDomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !PDT)
-    return;
-
-  // Only apply updates not are applied by PostDomTree.
-  if (hasPendingPostDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendPDTUpdateIndex;
-    const auto E = PendUpdates.end();
-    assert(I < E &&
-           "Iterator range invalid; there should be PostDomTree updates.");
-    PDT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendPDTUpdateIndex = PendUpdates.size();
+    if (!I->IsCriticalEdgeSplit) {
+      SmallVector<UpdateT, 32> NormalUpdates;
+      for (; I != E && !I->IsCriticalEdgeSplit; ++I)
+        NormalUpdates.push_back(I->Update);
+      DomTree->applyUpdates(NormalUpdates);
+      PendUpdateIndex += NormalUpdates.size();
+    } else {
+      SmallVector<CriticalEdge> CriticalEdges;
+      for (; I != E && I->IsCriticalEdgeSplit; ++I)
+        CriticalEdges.push_back(I->EdgeSplit);
+      IsForward ? splitDTCriticalEdges(CriticalEdges)
+                : splitPDTCriticalEdges(CriticalEdges);
+      PendUpdateIndex += CriticalEdges.size();
+    }
   }
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 bool GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::isUpdateValid(
-    typename DomTreeT::UpdateType Update) const {
+    UpdateT Update) const {
   const auto *From = Update.getFrom();
   const auto *To = Update.getTo();
   const auto Kind = Update.getKind();
@@ -347,6 +370,96 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT,
   PendPDTUpdateIndex -= dropIndex;
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!DT || Edges.empty())
+    return;
+
+  // 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 Edges.
+  // I.e., forall elt in Edges, it exists BB in NewBBs
+  // such as BB == elt.NewBB.
+  SmallSet<BasicBlockT *, 32> NewBBs;
+  for (auto &Edge : Edges)
+    NewBBs.insert(Edge.NewBB);
+  // For each element in Edges, 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 Edges is
+  // the ith element of IsNewIDom.
+  SmallBitVector IsNewIDom(Edges.size(), true);
+
+  // Collect all the dominance properties info, before invalidating
+  // the underlying DT.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // Update dominator information.
+    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.contains(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;
+      }
+    }
+  }
+
+  // Now, update DT with the collected dominance properties info.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // 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);
+  }
+}
+
+// Post dominator tree is different, the new basic block in critical edge
+// may become the new root.
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitPDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!PDT || Edges.empty())
+    return;
+
+  std::vector<UpdateT> Updates;
+  for (const auto &Edge : Edges) {
+    Updates.push_back({PostDomTreeT::Insert, Edge.FromBB, Edge.NewBB});
+    Updates.push_back({PostDomTreeT::Insert, Edge.NewBB, Edge.ToBB});
+    if (!llvm::is_contained(successors(Edge.FromBB), Edge.ToBB))
+      Updates.push_back({PostDomTreeT::Delete, Edge.FromBB, Edge.ToBB});
+  }
+  PDT->applyUpdates(Updates);
+}
+
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_GENERICDOMTREEUPDATERIMPL_H
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 6cf151c951b19f4..7fe33c3913f2dd4 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -32,6 +32,7 @@
 namespace llvm {
 
 class BasicBlock;
+class MachineDomTreeUpdater;
 class MachineFunction;
 class MCSymbol;
 class ModuleSlotTracker;
@@ -972,22 +973,23 @@ 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);
   }
 
   // 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);
 
   /// Check if the edge between this block and the given successor \p
   /// Succ, can be split. If this returns true a subsequent call to
@@ -1375,6 +1377,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
diff --git a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
index 9e3971f0b9fcecc..fcdc0becf31c12b 100644
--- a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
+++ b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
@@ -69,5 +69,12 @@ extern template void
 GenericDomTreeUpdater<MachineDomTreeUpdater, MachineDominatorTree,
                       MachinePostDominatorTree>::recalculate(MachineFunction
                                                                  &MF);
+
+extern template void GenericDomTreeUpdater<
+    Ma...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Dec 11, 2024

@llvm/pr-subscribers-llvm-regalloc

Author: None (paperchalice)

Changes

This reverts commit a28d49b7b54b85b4b2e344a0d4ebc9d511562fee.
When splitting critical edges, the post dominator tree may change its root node, and setNewRoot only works in normal dominator tree...

DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
assert(getNode(BB) == nullptr && "Block already in dominator tree!");
assert(!this->isPostDominator() &&
"Cannot change root of post-dominator tree");


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

30 Files Affected:

  • (modified) llvm/include/llvm/Analysis/DomTreeUpdater.h (+10-3)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdater.h (+36-7)
  • (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h (+170-57)
  • (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+16-8)
  • (modified) llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h (+7)
  • (modified) llvm/include/llvm/CodeGen/MachineDominators.h (+2-167)
  • (modified) llvm/include/llvm/CodeGen/MachineSSAContext.h (-6)
  • (modified) llvm/lib/Analysis/DomTreeUpdater.cpp (+7)
  • (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 (+5-3)
  • (modified) llvm/lib/CodeGen/MachineDomTreeUpdater.cpp (+7)
  • (modified) llvm/lib/CodeGen/MachineDominanceFrontier.cpp (+1-2)
  • (modified) llvm/lib/CodeGen/MachineDominators.cpp (-74)
  • (modified) llvm/lib/CodeGen/MachineLICM.cpp (+10-7)
  • (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 (+18-9)
  • (modified) llvm/lib/CodeGen/XRayInstrumentation.cpp (+2-2)
  • (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/Analysis/DomTreeUpdaterTest.cpp (+43)
  • (modified) llvm/unittests/Target/WebAssembly/WebAssemblyExceptionInfoTest.cpp (+4-4)
diff --git a/llvm/include/llvm/Analysis/DomTreeUpdater.h b/llvm/include/llvm/Analysis/DomTreeUpdater.h
index c120a6cc6ce5abe..0386262ba2b653e 100644
--- a/llvm/include/llvm/Analysis/DomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/DomTreeUpdater.h
@@ -81,6 +81,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:
@@ -109,9 +112,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,
@@ -120,6 +120,13 @@ extern template class GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
 extern template void
 GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
                       PostDominatorTree>::recalculate(Function &F);
+
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/true>();
+extern template void
+GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>::
+    applyUpdatesImpl</*IsForward=*/false>();
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_DOMTREEUPDATER_H
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
index ca4ce68b85cbcfc..4a03f548823ee47 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -30,6 +30,7 @@ class GenericDomTreeUpdater {
 public:
   enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };
   using BasicBlockT = typename DomTreeT::NodeType;
+  using UpdateT = typename DomTreeT::UpdateType;
 
   explicit GenericDomTreeUpdater(UpdateStrategy Strategy_)
       : Strategy(Strategy_) {}
@@ -146,7 +147,12 @@ 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.
-  void applyUpdates(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdates(ArrayRef<UpdateT> Updates);
+
+  /// Apply updates that the critical edge (FromBB, ToBB) has been
+  /// split with NewBB.
+  void splitCriticalEdge(BasicBlockT *FromBB, BasicBlockT *ToBB,
+                         BasicBlockT *NewBB);
 
   /// Submit updates to all available trees. It will also
   /// 1. discard duplicated updates,
@@ -169,7 +175,7 @@ class GenericDomTreeUpdater {
   /// 3. It is only legal to submit updates to an edge in the order CFG changes
   /// are made. The order you submit updates on different edges is not
   /// restricted.
-  void applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates);
+  void applyUpdatesPermissive(ArrayRef<UpdateT> Updates);
 
   ///@}
 
@@ -205,7 +211,25 @@ class GenericDomTreeUpdater {
   LLVM_DUMP_METHOD void dump() const;
 
 protected:
-  SmallVector<typename DomTreeT::UpdateType, 16> PendUpdates;
+  /// 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;
+  };
+
+  struct DomTreeUpdate {
+    bool IsCriticalEdgeSplit = false;
+    union {
+      UpdateT Update;
+      CriticalEdge EdgeSplit;
+    };
+    DomTreeUpdate(UpdateT Update) : Update(Update) {}
+    DomTreeUpdate(CriticalEdge E) : IsCriticalEdgeSplit(true), EdgeSplit(E) {}
+  };
+
+  SmallVector<DomTreeUpdate, 16> PendUpdates;
   size_t PendDTUpdateIndex = 0;
   size_t PendPDTUpdateIndex = 0;
   DomTreeT *DT = nullptr;
@@ -216,21 +240,21 @@ class GenericDomTreeUpdater {
   bool IsRecalculatingPostDomTree = false;
 
   /// Returns true if the update is self dominance.
-  bool isSelfDominance(typename DomTreeT::UpdateType Update) const {
+  bool isSelfDominance(UpdateT Update) const {
     // Won't affect DomTree and PostDomTree.
     return Update.getFrom() == Update.getTo();
   }
 
   /// Helper function to apply all pending DomTree updates.
-  void applyDomTreeUpdates();
+  void applyDomTreeUpdates() { applyUpdatesImpl<true>(); }
 
   /// Helper function to apply all pending PostDomTree updates.
-  void applyPostDomTreeUpdates();
+  void applyPostDomTreeUpdates() { applyUpdatesImpl<false>(); }
 
   /// Returns true if the update appears in the LLVM IR.
   /// It is used to check whether an update is valid in
   /// insertEdge/deleteEdge or is unnecessary in the batch update.
-  bool isUpdateValid(typename DomTreeT::UpdateType Update) const;
+  bool isUpdateValid(UpdateT Update) const;
 
   /// Erase Basic Block node before it is unlinked from Function
   /// in the DomTree and PostDomTree.
@@ -243,6 +267,11 @@ class GenericDomTreeUpdater {
   /// Drop all updates applied by all available trees and delete BasicBlocks if
   /// all available trees are up-to-date.
   void dropOutOfDateUpdates();
+
+private:
+  void splitDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  void splitPDTCriticalEdges(ArrayRef<CriticalEdge> Updates);
+  template <bool IsForward> void applyUpdatesImpl();
 };
 
 } // namespace llvm
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
index b79eaef57710fa8..896b68c5021b3b0 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdaterImpl.h
@@ -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"
@@ -56,7 +57,7 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::recalculate(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
-    ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
@@ -77,12 +78,12 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::applyUpdates(
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
-    applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    applyUpdatesPermissive(ArrayRef<UpdateT> Updates) {
   if (!DT && !PDT)
     return;
 
   SmallSet<std::pair<BasicBlockT *, BasicBlockT *>, 8> Seen;
-  SmallVector<typename DomTreeT::UpdateType, 8> DeduplicatedUpdates;
+  SmallVector<UpdateT, 8> DeduplicatedUpdates;
   for (const auto &U : Updates) {
     auto Edge = std::make_pair(U.getFrom(), U.getTo());
     // Because it is illegal to submit updates that have already been applied
@@ -129,6 +130,24 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
     PDT->applyUpdates(DeduplicatedUpdates);
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::splitCriticalEdge(
+    BasicBlockT *FromBB, BasicBlockT *ToBB, BasicBlockT *NewBB) {
+  if (!DT && !PDT)
+    return;
+
+  CriticalEdge E = {FromBB, ToBB, NewBB};
+  if (Strategy == UpdateStrategy::Lazy) {
+    PendUpdates.push_back(E);
+    return;
+  }
+
+  if (DT)
+    splitDTCriticalEdges(E);
+  if (PDT)
+    splitPDTCriticalEdges(E);
+}
+
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 DomTreeT &
 GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::getDomTree() {
@@ -171,39 +190,40 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     OS << "Lazy\n";
   int Index = 0;
 
+  auto printBlockInfo = [&](BasicBlockT *BB, StringRef Ending) {
+    if (BB) {
+      auto S = BB->getName();
+      if (!BB->hasName())
+        S = "(no name)";
+      OS << S << "(" << BB << ")" << Ending;
+    } else {
+      OS << "(badref)" << Ending;
+    }
+  };
+
   auto printUpdates =
-      [&](typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              begin,
-          typename ArrayRef<typename DomTreeT::UpdateType>::const_iterator
-              end) {
+      [&](typename ArrayRef<DomTreeUpdate>::const_iterator begin,
+          typename ArrayRef<DomTreeUpdate>::const_iterator end) {
         if (begin == end)
           OS << "  None\n";
         Index = 0;
         for (auto It = begin, ItEnd = end; It != ItEnd; ++It) {
-          auto U = *It;
-          OS << "  " << Index << " : ";
-          ++Index;
-          if (U.getKind() == DomTreeT::Insert)
-            OS << "Insert, ";
-          else
-            OS << "Delete, ";
-          BasicBlockT *From = U.getFrom();
-          if (From) {
-            auto S = From->getName();
-            if (!From->hasName())
-              S = "(no name)";
-            OS << S << "(" << From << "), ";
-          } else {
-            OS << "(badref), ";
-          }
-          BasicBlockT *To = U.getTo();
-          if (To) {
-            auto S = To->getName();
-            if (!To->hasName())
-              S = "(no_name)";
-            OS << S << "(" << To << ")\n";
+          if (!It->IsCriticalEdgeSplit) {
+            auto U = It->Update;
+            OS << "  " << Index << " : ";
+            ++Index;
+            if (U.getKind() == DomTreeT::Insert)
+              OS << "Insert, ";
+            else
+              OS << "Delete, ";
+            printBlockInfo(U.getFrom(), ", ");
+            printBlockInfo(U.getTo(), "\n");
           } else {
-            OS << "(badref)\n";
+            const auto &Edge = It->EdgeSplit;
+            OS << "  " << Index++ << " : Split critical edge, ";
+            printBlockInfo(Edge.FromBB, ", ");
+            printBlockInfo(Edge.ToBB, ", ");
+            printBlockInfo(Edge.NewBB, "\n");
           }
         }
       };
@@ -236,50 +256,53 @@ GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::dump() const {
     if (BB->hasName())
       OS << BB->getName() << "(";
     else
-      OS << "(no_name)(";
+      OS << "(no name)(";
     OS << BB << ")\n";
   }
 #endif
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+template <bool IsForward>
 void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyDomTreeUpdates() {
+                           PostDomTreeT>::applyUpdatesImpl() {
+  auto *DomTree = [&]() {
+    if constexpr (IsForward)
+      return DT;
+    else
+      return PDT;
+  }();
   // No pending DomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !DT)
+  if (Strategy != UpdateStrategy::Lazy || !DomTree)
     return;
+  size_t &PendUpdateIndex = IsForward ? PendDTUpdateIndex : PendPDTUpdateIndex;
 
-  // Only apply updates not are applied by DomTree.
-  if (hasPendingDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendDTUpdateIndex;
+  // Only apply updates not are applied by (Post)DomTree.
+  while (IsForward ? hasPendingDomTreeUpdates()
+                   : hasPendingPostDomTreeUpdates()) {
+    auto I = PendUpdates.begin() + PendUpdateIndex;
     const auto E = PendUpdates.end();
     assert(I < E && "Iterator range invalid; there should be DomTree updates.");
-    DT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendDTUpdateIndex = PendUpdates.size();
-  }
-}
-
-template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
-void GenericDomTreeUpdater<DerivedT, DomTreeT,
-                           PostDomTreeT>::applyPostDomTreeUpdates() {
-  // No pending PostDomTreeUpdates.
-  if (Strategy != UpdateStrategy::Lazy || !PDT)
-    return;
-
-  // Only apply updates not are applied by PostDomTree.
-  if (hasPendingPostDomTreeUpdates()) {
-    const auto I = PendUpdates.begin() + PendPDTUpdateIndex;
-    const auto E = PendUpdates.end();
-    assert(I < E &&
-           "Iterator range invalid; there should be PostDomTree updates.");
-    PDT->applyUpdates(ArrayRef<typename DomTreeT::UpdateType>(I, E));
-    PendPDTUpdateIndex = PendUpdates.size();
+    if (!I->IsCriticalEdgeSplit) {
+      SmallVector<UpdateT, 32> NormalUpdates;
+      for (; I != E && !I->IsCriticalEdgeSplit; ++I)
+        NormalUpdates.push_back(I->Update);
+      DomTree->applyUpdates(NormalUpdates);
+      PendUpdateIndex += NormalUpdates.size();
+    } else {
+      SmallVector<CriticalEdge> CriticalEdges;
+      for (; I != E && I->IsCriticalEdgeSplit; ++I)
+        CriticalEdges.push_back(I->EdgeSplit);
+      IsForward ? splitDTCriticalEdges(CriticalEdges)
+                : splitPDTCriticalEdges(CriticalEdges);
+      PendUpdateIndex += CriticalEdges.size();
+    }
   }
 }
 
 template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
 bool GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::isUpdateValid(
-    typename DomTreeT::UpdateType Update) const {
+    UpdateT Update) const {
   const auto *From = Update.getFrom();
   const auto *To = Update.getTo();
   const auto Kind = Update.getKind();
@@ -347,6 +370,96 @@ void GenericDomTreeUpdater<DerivedT, DomTreeT,
   PendPDTUpdateIndex -= dropIndex;
 }
 
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!DT || Edges.empty())
+    return;
+
+  // 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 Edges.
+  // I.e., forall elt in Edges, it exists BB in NewBBs
+  // such as BB == elt.NewBB.
+  SmallSet<BasicBlockT *, 32> NewBBs;
+  for (auto &Edge : Edges)
+    NewBBs.insert(Edge.NewBB);
+  // For each element in Edges, 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 Edges is
+  // the ith element of IsNewIDom.
+  SmallBitVector IsNewIDom(Edges.size(), true);
+
+  // Collect all the dominance properties info, before invalidating
+  // the underlying DT.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // Update dominator information.
+    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.contains(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;
+      }
+    }
+  }
+
+  // Now, update DT with the collected dominance properties info.
+  for (const auto &[Idx, Edge] : enumerate(Edges)) {
+    // 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);
+  }
+}
+
+// Post dominator tree is different, the new basic block in critical edge
+// may become the new root.
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+void GenericDomTreeUpdater<DerivedT, DomTreeT, PostDomTreeT>::
+    splitPDTCriticalEdges(ArrayRef<CriticalEdge> Edges) {
+  // Bail out early if there is nothing to do.
+  if (!PDT || Edges.empty())
+    return;
+
+  std::vector<UpdateT> Updates;
+  for (const auto &Edge : Edges) {
+    Updates.push_back({PostDomTreeT::Insert, Edge.FromBB, Edge.NewBB});
+    Updates.push_back({PostDomTreeT::Insert, Edge.NewBB, Edge.ToBB});
+    if (!llvm::is_contained(successors(Edge.FromBB), Edge.ToBB))
+      Updates.push_back({PostDomTreeT::Delete, Edge.FromBB, Edge.ToBB});
+  }
+  PDT->applyUpdates(Updates);
+}
+
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_GENERICDOMTREEUPDATERIMPL_H
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 6cf151c951b19f4..7fe33c3913f2dd4 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -32,6 +32,7 @@
 namespace llvm {
 
 class BasicBlock;
+class MachineDomTreeUpdater;
 class MachineFunction;
 class MCSymbol;
 class ModuleSlotTracker;
@@ -972,22 +973,23 @@ 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);
   }
 
   // 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);
 
   /// Check if the edge between this block and the given successor \p
   /// Succ, can be split. If this returns true a subsequent call to
@@ -1375,6 +1377,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
diff --git a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
index 9e3971f0b9fcecc..fcdc0becf31c12b 100644
--- a/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
+++ b/llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h
@@ -69,5 +69,12 @@ extern template void
 GenericDomTreeUpdater<MachineDomTreeUpdater, MachineDominatorTree,
                       MachinePostDominatorTree>::recalculate(MachineFunction
                                                                  &MF);
+
+extern template void GenericDomTreeUpdater<
+    Ma...
[truncated]

@qcolombet
Copy link
Collaborator

qcolombet commented Dec 11, 2024

This reverts commit a28d49b7b54b85b4b2e344a0d4ebc9d511562fee. Use traditional way to update post dominator tree. When splitting critical edges, the post dominator tree may change its root node, and setNewRoot only works in normal dominator tree...

DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
assert(getNode(BB) == nullptr && "Block already in dominator tree!");
assert(!this->isPostDominator() &&
"Cannot change root of post-dominator tree");

What did you change in the new version of the patch to avoid this problem?

@paperchalice
Copy link
Contributor Author

paperchalice commented Dec 11, 2024

This reverts commit a28d49b7b54b85b4b2e344a0d4ebc9d511562fee. Use traditional way to update post dominator tree. When splitting critical edges, the post dominator tree may change its root node, and setNewRoot only works in normal dominator tree...

DomTreeNodeBase<NodeT> *setNewRoot(NodeT *BB) {
assert(getNode(BB) == nullptr && "Block already in dominator tree!");
assert(!this->isPostDominator() &&
"Cannot change root of post-dominator tree");

What did you change in the new version of the patch to avoid this problem?

Convert critical edge splitting to normal updates and use applyUpdates for post dominator tree in splitPDTCriticalEdges...

@paperchalice
Copy link
Contributor Author

I tried to modify post dominator tree via following code but failed with changing the root node.

for (const auto &[Idx, Edge] : enumerate(Edges)) {
    auto *FromNode = PDT->getNode(Edge.FromBB);

    // If FromBB is a non trivial root, then NewBB is the new non trivial root,
    // and NewBB post dominates FromBB.
    if (PDT->isVirtualRoot(FromNode->getIDom()) && !successors(Edge.FromBB).empty()) {
      auto *NewPDTNode = PDT->addNewBlock(Edge.NewBB, nullptr);
      PDT->changeImmediateDominator(FromNode, NewPDTNode);
      // PDT->setNewRoot(NewPDTNode); // Impossible...
    } else {
      // ToBB post dominates NewBB
      auto *NewPDTNode = PDT->addNewBlock(Edge.NewBB, Edge.ToBB);
      if (IsNewIPDom[Idx])
        PDT->changeImmediateDominator(FromNode, NewPDTNode);
    }
  }

@paperchalice paperchalice merged commit 1562b70 into llvm:main Dec 13, 2024
16 checks passed
@paperchalice paperchalice linked an issue Dec 13, 2024 that may be closed by this pull request
@qcolombet
Copy link
Collaborator

I tried to modify post dominator tree via following code but failed with changing the root node.

for (const auto &[Idx, Edge] : enumerate(Edges)) {
    auto *FromNode = PDT->getNode(Edge.FromBB);

    // If FromBB is a non trivial root, then NewBB is the new non trivial root,
    // and NewBB post dominates FromBB.
    if (PDT->isVirtualRoot(FromNode->getIDom()) && !successors(Edge.FromBB).empty()) {
      auto *NewPDTNode = PDT->addNewBlock(Edge.NewBB, nullptr);
      PDT->changeImmediateDominator(FromNode, NewPDTNode);
      // PDT->setNewRoot(NewPDTNode); // Impossible...
    } else {
      // ToBB post dominates NewBB
      auto *NewPDTNode = PDT->addNewBlock(Edge.NewBB, Edge.ToBB);
      if (IsNewIPDom[Idx])
        PDT->changeImmediateDominator(FromNode, NewPDTNode);
    }
  }

Oh I see. By normal updates you meant not trying to be smart about it. I thought we were losing the laziness of the updater, hence my confusion.

@paperchalice paperchalice deleted the reapply-dt branch December 14, 2024 07:53
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.

[MachineSink] llc hang caused by #115111
3 participants