[BBUtils][NFC] Delete SplitLandingPadPredecessors with DT

[caojoshua]

Function is marked for deprecation. There is only one consumer which is converted to use DomTreeUpdater.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: Joshua Cao (caojoshua)

Changes

Function is marked for deprecation. There is only one consumer which is converted to use DomTreeUpdater.

---

Full diff: https://github.com/llvm/llvm-project/pull/73406.diff

3 Files Affected:

• (modified) llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h (-21)
• (modified) llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp (+3-1)
• (modified) llvm/lib/Transforms/Utils/BasicBlockUtils.cpp (-11)

diff --git a/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h b/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h
index e6dde450b7df9c8..88e3b4b628b8e9b 100644
--- a/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h
@@ -367,27 +367,6 @@ BasicBlock *SplitBlockPredecessors(BasicBlock *BB, ArrayRef<BasicBlock *> Preds,
                                    MemorySSAUpdater *MSSAU = nullptr,
                                    bool PreserveLCSSA = false);
 
-/// This method transforms the landing pad, OrigBB, by introducing two new basic
-/// blocks into the function. One of those new basic blocks gets the
-/// predecessors listed in Preds. The other basic block gets the remaining
-/// predecessors of OrigBB. The landingpad instruction OrigBB is clone into both
-/// of the new basic blocks. The new blocks are given the suffixes 'Suffix1' and
-/// 'Suffix2', and are returned in the NewBBs vector.
-///
-/// This currently updates the LLVM IR, DominatorTree, LoopInfo, and LCCSA but
-/// no other analyses. In particular, it does not preserve LoopSimplify
-/// (because it's complicated to handle the case where one of the edges being
-/// split is an exit of a loop with other exits).
-///
-/// FIXME: deprecated, switch to the DomTreeUpdater-based one.
-void SplitLandingPadPredecessors(BasicBlock *OrigBB,
-                                 ArrayRef<BasicBlock *> Preds,
-                                 const char *Suffix, const char *Suffix2,
-                                 SmallVectorImpl<BasicBlock *> &NewBBs,
-                                 DominatorTree *DT, LoopInfo *LI = nullptr,
-                                 MemorySSAUpdater *MSSAU = nullptr,
-                                 bool PreserveLCSSA = false);
-
 /// This method transforms the landing pad, OrigBB, by introducing two new basic
 /// blocks into the function. One of those new basic blocks gets the
 /// predecessors listed in Preds. The other basic block gets the remaining
diff --git a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
index 6e04f54e13da547..c15cda975169e77 100644
--- a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
@@ -67,6 +67,7 @@
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/DomTreeUpdater.h"
 #include "llvm/Analysis/IVUsers.h"
 #include "llvm/Analysis/LoopAnalysisManager.h"
 #include "llvm/Analysis/LoopInfo.h"
@@ -5599,7 +5600,8 @@ void LSRInstance::RewriteForPHI(
                                       .setKeepOneInputPHIs());
           } else {
             SmallVector<BasicBlock*, 2> NewBBs;
-            SplitLandingPadPredecessors(Parent, BB, "", "", NewBBs, &DT, &LI);
+            DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager);
+            SplitLandingPadPredecessors(Parent, BB, "", "", NewBBs, &DTU, &LI);
             NewBB = NewBBs[0];
           }
           // If NewBB==NULL, then SplitCriticalEdge refused to split because all
diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 168998fbee114ab..3840aeb9eba2199 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -1452,17 +1452,6 @@ static void SplitLandingPadPredecessorsImpl(
   }
 }
 
-void llvm::SplitLandingPadPredecessors(BasicBlock *OrigBB,
-                                       ArrayRef<BasicBlock *> Preds,
-                                       const char *Suffix1, const char *Suffix2,
-                                       SmallVectorImpl<BasicBlock *> &NewBBs,
-                                       DominatorTree *DT, LoopInfo *LI,
-                                       MemorySSAUpdater *MSSAU,
-                                       bool PreserveLCSSA) {
-  return SplitLandingPadPredecessorsImpl(
-      OrigBB, Preds, Suffix1, Suffix2, NewBBs,
-      /*DTU=*/nullptr, DT, LI, MSSAU, PreserveLCSSA);
-}
 void llvm::SplitLandingPadPredecessors(BasicBlock *OrigBB,
                                        ArrayRef<BasicBlock *> Preds,
                                        const char *Suffix1, const char *Suffix2,

[caojoshua]

original review on phabricator https://reviews.llvm.org/D148780

[nikic]

Adding @kuhar here to get an opinion on the general direction. We have a bunch of APIs in BBUtils that support both DT and DTU. The UpdateAnalysisInformation() handles the actual updates using either DT or DTU.

To what degree is it our goal to switch updates directly on DT to use DTU, in places that don't otherwise use DTU? My understanding is that the updates directly on DT will be more efficient than going through the generic DTU implementation, so I'm not really clear on whether this kind of change is desirable or not.

[kuhar]

Adding @kuhar here to get an opinion on the general direction. We have a bunch of APIs in BBUtils that support both DT and DTU. The UpdateAnalysisInformation() handles the actual updates using either DT or DTU.

To what degree is it our goal to switch updates directly on DT to use DTU, in places that don't otherwise use DTU? My understanding is that the updates directly on DT will be more efficient than going through the generic DTU implementation, so I'm not really clear on whether this kind of change is desirable or not.

DTU makes it worth it when we have a sequence of updates and some of them may cancel out or we may realize it's not worth doing incremental updates and all and rebuild the tree instead. With a local DTU that applies updates immediately, there's little benefit except defaulting to APIs that allow to chain more updates in the future.

Overall, my heuristic would be:

• API --> DTU
• Chaining updates --> DTU
• Local updates: not much difference, either is fine, but DT would constitute fewer stack frames if you ever have to debug this