Port PlaceSafepoints pass to the new pass manager

[jandupej]

Summary

This patch ports the PlaceSafepoints pass to the new pass manager as it is used by .NET/Mono. Compatibility with the legacy pass manager is maintained by adding PlaceSafepointsLegacyPass. This pass also depends on PlaceBackedgeSafepointsLegacyPass, which has been kept in the legacy-only variant, since it is apparently used only from PlaceSafepointsPass. It has been renamed, though, to indicate its legacy interface.

[aeubanks]

looks fine, mostly just some nits

llvm/include/llvm/Transforms/Scalar/PlaceSafepoints.h

60

is this necessary?

74

nit: should have newline at end of file

llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp

101

not sure what this is for, but it's not used elsewhere in LLVM so it probably shouldn't be added here

llvm/test/Transforms/PlaceSafepoints/split-backedge.ll

2

-passes=place-safepoints

[speryt]

llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp

101

This is most likely something specific to Visual Studio editor. There is short comment about that in clang/lib/Lex/Pragma.cpp

/// Handle "\#pragma region [...]"
///
/// The syntax is
/// \code
///   #pragma region [optional name]
///   #pragma endregion [optional comment]
/// \endcode
///
/// \note This is
/// <a href="http://msdn.microsoft.com/en-us/library/b6xkz944(v=vs.80).aspx">editor-only</a>
/// pragma, just skipped by compiler.
struct PragmaRegionHandler : public PragmaHandler

So it seems it's safe to drop those.

[aeubanks]

do you need somebody to land this for you?

llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp

342

this should be updated to not use the legacy pm in a followup change

[jandupej]

In D136163#4129538, @aeubanks wrote:

do you need somebody to land this for you?

That would be great!

[aeubanks]

In D136163#4131325, @jandupej wrote:

In D136163#4129538, @aeubanks wrote:

do you need somebody to land this for you?

That would be great!

is there an email you'd like to use for the git commit?

[jandupej]

In D136163#4132917, @aeubanks wrote:

In D136163#4131325, @jandupej wrote:

In D136163#4129538, @aeubanks wrote:

do you need somebody to land this for you?

That would be great!

is there an email you'd like to use for the git commit?

[e-mail]

llvm/include/llvm/InitializePasses.h

 void initializeOptimizePHIsPass(PassRegistry&);
 void initializePEIPass(PassRegistry&);
 void initializePHIEliminationPass(PassRegistry&);
 void initializePartiallyInlineLibCallsLegacyPassPass(PassRegistry&);
 void initializePatchableFunctionPass(PassRegistry&);
 void initializePeepholeOptimizerPass(PassRegistry&);
 void initializePhiValuesWrapperPassPass(PassRegistry&);
 void initializePhysicalRegisterUsageInfoPass(PassRegistry&);
-void initializePlaceBackedgeSafepointsImplPass(PassRegistry&);
-void initializePlaceSafepointsPass(PassRegistry&);
+void initializePlaceBackedgeSafepointsLegacyPassPass(PassRegistry &);
+void initializePlaceSafepointsLegacyPassPass(PassRegistry &);
 void initializePostDomOnlyPrinterWrapperPassPass(PassRegistry &);
 void initializePostDomOnlyViewerWrapperPassPass(PassRegistry &);
 void initializePostDomPrinterWrapperPassPass(PassRegistry &);
 void initializePostDomViewerWrapperPassPass(PassRegistry &);
 void initializePostDominatorTreeWrapperPassPass(PassRegistry&);
 void initializePostMachineSchedulerPass(PassRegistry&);
 void initializePostOrderFunctionAttrsLegacyPassPass(PassRegistry&);
 void initializePostRAHazardRecognizerPass(PassRegistry&);

llvm/include/llvm/Transforms/Scalar/PlaceSafepoints.h

+//===- PlaceSafepoints.h - Place GC Safepoints ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Place garbage collection safepoints at appropriate locations in the IR. This
+// does not make relocation semantics or variable liveness explicit.  That's
+// done by RewriteStatepointsForGC.
+//
+// Terminology:
+// - A call is said to be "parseable" if there is a stack map generated for the
+// return PC of the call.  A runtime can determine where values listed in the
+// deopt arguments and (after RewriteStatepointsForGC) gc arguments are located
+// on the stack when the code is suspended inside such a call.  Every parse
+// point is represented by a call wrapped in an gc.statepoint intrinsic.
+// - A "poll" is an explicit check in the generated code to determine if the
+// runtime needs the generated code to cooperate by calling a helper routine
+// and thus suspending its execution at a known state. The call to the helper
+// routine will be parseable.  The (gc & runtime specific) logic of a poll is
+// assumed to be provided in a function of the name "gc.safepoint_poll".
+//
+// We aim to insert polls such that running code can quickly be brought to a
+// well defined state for inspection by the collector.  In the current
+// implementation, this is done via the insertion of poll sites at method entry
+// and the backedge of most loops.  We try to avoid inserting more polls than
+// are necessary to ensure a finite period between poll sites.  This is not
+// because the poll itself is expensive in the generated code; it's not.  Polls
+// do tend to impact the optimizer itself in negative ways; we'd like to avoid
+// perturbing the optimization of the method as much as we can.
+//
+// We also need to make most call sites parseable.  The callee might execute a
+// poll (or otherwise be inspected by the GC).  If so, the entire stack
+// (including the suspended frame of the current method) must be parseable.
+//
+// This pass will insert:
+// - Call parse points ("call safepoints") for any call which may need to
+// reach a safepoint during the execution of the callee function.
+// - Backedge safepoint polls and entry safepoint polls to ensure that
+// executing code reaches a safepoint poll in a finite amount of time.
+//
+// We do not currently support return statepoints, but adding them would not
+// be hard.  They are not required for correctness - entry safepoints are an
+// alternative - but some GCs may prefer them.  Patches welcome.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TRANSFORMS_SCALAR_PLACESAFEPOINTS_H
+#define LLVM_TRANSFORMS_SCALAR_PLACESAFEPOINTS_H
+
+#include "llvm/IR/PassManager.h"
+
+namespace llvm {
+
+class TargetLibraryInfo;
+
+class PlaceSafepointsPass : public PassInfoMixin<PlaceSafepointsPass> {
+public:
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+
+  bool runImpl(Function &F, const TargetLibraryInfo &TLI);
+
+  void cleanup() {}
+
+private:
+};
+} // namespace llvm
+
+#endif // LLVM_TRANSFORMS_SCALAR_PLACESAFEPOINTS_H

llvm/lib/Passes/PassBuilder.cpp

 #include "llvm/Transforms/Scalar/LowerWidenableCondition.h"
 #include "llvm/Transforms/Scalar/MakeGuardsExplicit.h"
 #include "llvm/Transforms/Scalar/MemCpyOptimizer.h"
 #include "llvm/Transforms/Scalar/MergeICmps.h"
 #include "llvm/Transforms/Scalar/MergedLoadStoreMotion.h"
 #include "llvm/Transforms/Scalar/NaryReassociate.h"
 #include "llvm/Transforms/Scalar/NewGVN.h"
 #include "llvm/Transforms/Scalar/PartiallyInlineLibCalls.h"
+#include "llvm/Transforms/Scalar/PlaceSafepoints.h"
 #include "llvm/Transforms/Scalar/Reassociate.h"
 #include "llvm/Transforms/Scalar/Reg2Mem.h"
 #include "llvm/Transforms/Scalar/RewriteStatepointsForGC.h"
 #include "llvm/Transforms/Scalar/SCCP.h"
 #include "llvm/Transforms/Scalar/SROA.h"
 #include "llvm/Transforms/Scalar/ScalarizeMaskedMemIntrin.h"
 #include "llvm/Transforms/Scalar/Scalarizer.h"
 #include "llvm/Transforms/Scalar/SeparateConstOffsetFromGEP.h"

llvm/lib/Passes/PassRegistry.def

 FUNCTION_PASS("loop-fusion", LoopFusePass())
 FUNCTION_PASS("loop-distribute", LoopDistributePass())
 FUNCTION_PASS("loop-versioning", LoopVersioningPass())
 FUNCTION_PASS("objc-arc", ObjCARCOptPass())
 FUNCTION_PASS("objc-arc-contract", ObjCARCContractPass())
 FUNCTION_PASS("objc-arc-expand", ObjCARCExpandPass())
 FUNCTION_PASS("pa-eval", PAEvalPass())
 FUNCTION_PASS("pgo-memop-opt", PGOMemOPSizeOpt())
+FUNCTION_PASS("place-safepoints", PlaceSafepointsPass())
 FUNCTION_PASS("print", PrintFunctionPass(dbgs()))
 FUNCTION_PASS("print<assumptions>", AssumptionPrinterPass(dbgs()))
 FUNCTION_PASS("print<block-freq>", BlockFrequencyPrinterPass(dbgs()))
 FUNCTION_PASS("print<branch-prob>", BranchProbabilityPrinterPass(dbgs()))
 FUNCTION_PASS("print<cost-model>", CostModelPrinterPass(dbgs()))
 FUNCTION_PASS("print<cycles>", CycleInfoPrinterPass(dbgs()))
 FUNCTION_PASS("print<da>", DependenceAnalysisPrinterPass(dbgs()))
 FUNCTION_PASS("print<divergence>", DivergenceAnalysisPrinterPass(dbgs()))

llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp

 // executing code reaches a safepoint poll in a finite amount of time.
 //
 // We do not currently support return statepoints, but adding them would not
 // be hard.  They are not required for correctness - entry safepoints are an
 // alternative - but some GCs may prefer them.  Patches welcome.
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/Transforms/Scalar/PlaceSafepoints.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/CFG.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/ScalarEvolution.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Statepoint.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Local.h"
 
-#define DEBUG_TYPE "safepoint-placement"
+using namespace llvm;
+
+#define DEBUG_TYPE "place-safepoints"
 
 STATISTIC(NumEntrySafepoints, "Number of entry safepoints inserted");
 STATISTIC(NumBackedgeSafepoints, "Number of backedge safepoints inserted");
 
 STATISTIC(CallInLoop,
           "Number of loops without safepoints due to calls in loop");
 STATISTIC(FiniteExecution,
           "Number of loops without safepoints finite execution");
 
-using namespace llvm;
-
 // Ignore opportunities to avoid placing safepoints on backedges, useful for
 // validation
 static cl::opt<bool> AllBackedges("spp-all-backedges", cl::Hidden,
                                   cl::init(false));
 
 /// How narrow does the trip count of a loop have to be to have to be considered
 /// "counted"?  Counted loops do not get safepoints at backedges.
 static cl::opt<int> CountedLoopTripWidth("spp-counted-loop-trip-width",
                                          cl::Hidden, cl::init(32));
 
 // If true, split the backedge of a loop when placing the safepoint, otherwise
 // split the latch block itself.  Both are useful to support for
 // experimentation, but in practice, it looks like splitting the backedge
 // optimizes better.
 static cl::opt<bool> SplitBackedge("spp-split-backedge", cl::Hidden,
                                    cl::init(false));
 
 namespace {
-
 /// An analysis pass whose purpose is to identify each of the backedges in
 /// the function which require a safepoint poll to be inserted.
-struct PlaceBackedgeSafepointsImpl : public FunctionPass {
+class PlaceBackedgeSafepointsLegacyPass : public FunctionPass {
+public:
   static char ID;
 
   /// The output of the pass - gives a list of each backedge (described by
   /// pointing at the branch) which need a poll inserted.
   std::vector<Instruction *> PollLocations;
 
   /// True unless we're running spp-no-calls in which case we need to disable
   /// the call-dependent placement opts.
   bool CallSafepointsEnabled;
 
-  ScalarEvolution *SE = nullptr;
-  DominatorTree *DT = nullptr;
-  LoopInfo *LI = nullptr;
-  TargetLibraryInfo *TLI = nullptr;
-
-  PlaceBackedgeSafepointsImpl(bool CallSafepoints = false)
+  PlaceBackedgeSafepointsLegacyPass(bool CallSafepoints = false)
       : FunctionPass(ID), CallSafepointsEnabled(CallSafepoints) {
-    initializePlaceBackedgeSafepointsImplPass(*PassRegistry::getPassRegistry());
+    initializePlaceBackedgeSafepointsLegacyPassPass(
+        *PassRegistry::getPassRegistry());
   }
 
   bool runOnLoop(Loop *);
+
   void runOnLoopAndSubLoops(Loop *L) {
     // Visit all the subloops
     for (Loop *I : *L)
       runOnLoopAndSubLoops(I);
     runOnLoop(L);
   }
 
   bool runOnFunction(Function &F) override {
void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<DominatorTreeWrapperPass>();
     AU.addRequired<ScalarEvolutionWrapperPass>();
     AU.addRequired<LoopInfoWrapperPass>();
     AU.addRequired<TargetLibraryInfoWrapperPass>();
     // We no longer modify the IR at all in this pass.  Thus all
     // analysis are preserved.
     AU.setPreservesAll();
   }
+
+private:
+  ScalarEvolution *SE = nullptr;
+  DominatorTree *DT = nullptr;
+  LoopInfo *LI = nullptr;
+  TargetLibraryInfo *TLI = nullptr;
 };
-}
+} // namespace
 
 static cl::opt<bool> NoEntry("spp-no-entry", cl::Hidden, cl::init(false));
 static cl::opt<bool> NoCall("spp-no-call", cl::Hidden, cl::init(false));
 static cl::opt<bool> NoBackedge("spp-no-backedge", cl::Hidden, cl::init(false));
 
+char PlaceBackedgeSafepointsLegacyPass::ID = 0;
+
+INITIALIZE_PASS_BEGIN(PlaceBackedgeSafepointsLegacyPass,
+                      "place-backedge-safepoints-impl",
+                      "Place Backedge Safepoints", false, false)
+INITIALIZE_PASS_DEPENDENCY(ScalarEvolutionWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
+INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
+INITIALIZE_PASS_END(PlaceBackedgeSafepointsLegacyPass,
+                    "place-backedge-safepoints-impl",
+                    "Place Backedge Safepoints", false, false)
+
+static bool containsUnconditionalCallSafepoint(Loop *L, BasicBlock *Header,
+                                               BasicBlock *Pred,
+                                               DominatorTree &DT,
+                                               const TargetLibraryInfo &TLI);
+
+static bool mustBeFiniteCountedLoop(Loop *L, ScalarEvolution *SE,
+                                    BasicBlock *Pred);
+
+static Instruction *findLocationForEntrySafepoint(Function &F,
+                                                  DominatorTree &DT);
+
+static bool isGCSafepointPoll(Function &F);
+static bool shouldRewriteFunction(Function &F);
+static bool enableEntrySafepoints(Function &F);
+static bool enableBackedgeSafepoints(Function &F);
+static bool enableCallSafepoints(Function &F);
+
+static void
+InsertSafepointPoll(Instruction *InsertBefore,
+                    std::vector<CallBase *> &ParsePointsNeeded /*rval*/,
+                    const TargetLibraryInfo &TLI);
+
+bool PlaceBackedgeSafepointsLegacyPass::runOnLoop(Loop *L) {
+  // Loop through all loop latches (branches controlling backedges).  We need
+  // to place a safepoint on every backedge (potentially).
+  // Note: In common usage, there will be only one edge due to LoopSimplify
+  // having run sometime earlier in the pipeline, but this code must be correct
+  // w.r.t. loops with multiple backedges.
+  BasicBlock *Header = L->getHeader();
+  SmallVector<BasicBlock *, 16> LoopLatches;
+  L->getLoopLatches(LoopLatches);
+  for (BasicBlock *Pred : LoopLatches) {
+    assert(L->contains(Pred));
+
+    // Make a policy decision about whether this loop needs a safepoint or
+    // not.  Note that this is about unburdening the optimizer in loops, not
+    // avoiding the runtime cost of the actual safepoint.
+    if (!AllBackedges) {
+      if (mustBeFiniteCountedLoop(L, SE, Pred)) {
+        LLVM_DEBUG(dbgs() << "skipping safepoint placement in finite loop\n");
+        FiniteExecution++;
+        continue;
+      }
+      if (CallSafepointsEnabled &&
+          containsUnconditionalCallSafepoint(L, Header, Pred, *DT, *TLI)) {
+        // Note: This is only semantically legal since we won't do any further
+        // IPO or inlining before the actual call insertion..  If we hadn't, we
+        // might latter loose this call safepoint.
+        LLVM_DEBUG(
+            dbgs()
+            << "skipping safepoint placement due to unconditional call\n");
+        CallInLoop++;
+        continue;
+      }
+    }
+
+    // TODO: We can create an inner loop which runs a finite number of
+    // iterations with an outer loop which contains a safepoint.  This would
+    // not help runtime performance that much, but it might help our ability to
+    // optimize the inner loop.
+
+    // Safepoint insertion would involve creating a new basic block (as the
+    // target of the current backedge) which does the safepoint (of all live
+    // variables) and branches to the true header
+    Instruction *Term = Pred->getTerminator();
+
+    LLVM_DEBUG(dbgs() << "[LSP] terminator instruction: " << *Term);
+
+    PollLocations.push_back(Term);
+  }
+
+  return false;
+}
+
 namespace {
-struct PlaceSafepoints : public FunctionPass {
+class PlaceSafepointsLegacyPass : public FunctionPass {
+public:
   static char ID; // Pass identification, replacement for typeid
 
-  PlaceSafepoints() : FunctionPass(ID) {
-    initializePlaceSafepointsPass(*PassRegistry::getPassRegistry());
+  PlaceSafepointsLegacyPass() : FunctionPass(ID) {
+    initializePlaceSafepointsLegacyPassPass(*PassRegistry::getPassRegistry());
   }
+
   bool runOnFunction(Function &F) override;
 
+  StringRef getPassName() const override { return "Safepoint Placement"; }
+
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     // We modify the graph wholesale (inlining, block insertion, etc).  We
     // preserve nothing at the moment.  We could potentially preserve dom tree
     // if that was worth doing
     AU.addRequired<TargetLibraryInfoWrapperPass>();
   }
+
+private:
+  PlaceSafepointsPass Impl;
 };
+} // end anonymous namespace
+
+char PlaceSafepointsLegacyPass::ID = 0;
+
+INITIALIZE_PASS_BEGIN(PlaceSafepointsLegacyPass, "place-safepoints",
+                      "Place Safepoints", false, false)
+INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
+INITIALIZE_PASS_END(PlaceSafepointsLegacyPass, "place-safepoints",
+                    "Place Safepoints", false, false)
+
+FunctionPass *llvm::createPlaceSafepointsPass() {
+  return new PlaceSafepointsLegacyPass();
 }
 
-// Insert a safepoint poll immediately before the given instruction.  Does
-// not handle the parsability of state at the runtime call, that's the
-// callers job.
-static void
-InsertSafepointPoll(Instruction *InsertBefore,
-                    std::vector<CallBase *> &ParsePointsNeeded /*rval*/,
-                    const TargetLibraryInfo &TLI);
+bool PlaceSafepointsLegacyPass::runOnFunction(Function &F) {
+  if (skipFunction(F))
+    return false;
+
+  LLVM_DEBUG(dbgs() << "********** Begin Safepoint Placement **********\n");
+  LLVM_DEBUG(dbgs() << "********** Function: " << F.getName() << '\n');
+
+  bool MadeChange =
+      Impl.runImpl(F, getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F));
+
+  if (MadeChange) {
+    LLVM_DEBUG(dbgs() << "********** Function after Safepoint Placement: "
+                      << F.getName() << '\n');
+    LLVM_DEBUG(dbgs() << F);
+  }
+  LLVM_DEBUG(dbgs() << "********** End Safepoint Placement **********\n");
+
+  return MadeChange;
+}
+
+bool PlaceSafepointsPass::runImpl(Function &F, const TargetLibraryInfo &TLI) {
+  if (F.isDeclaration() || F.empty()) {
+    // This is a declaration, nothing to do.  Must exit early to avoid crash in
+    // dom tree calculation
+    return false;
+  }
+
+  if (isGCSafepointPoll(F)) {
+    // Given we're inlining this inside of safepoint poll insertion, this
+    // doesn't make any sense.  Note that we do make any contained calls
+    // parseable after we inline a poll.
+    return false;
+  }
+
+  if (!shouldRewriteFunction(F))
+    return false;
+
+  bool Modified = false;
+
+  // In various bits below, we rely on the fact that uses are reachable from
+  // defs.  When there are basic blocks unreachable from the entry, dominance
+  // and reachablity queries return non-sensical results.  Thus, we preprocess
+  // the function to ensure these properties hold.
+  Modified |= removeUnreachableBlocks(F);
+
+  // STEP 1 - Insert the safepoint polling locations.  We do not need to
+  // actually insert parse points yet.  That will be done for all polls and
+  // calls in a single pass.
+
+  DominatorTree DT;
+  DT.recalculate(F);
+
+  SmallVector<Instruction *, 16> PollsNeeded;
+  std::vector<CallBase *> ParsePointNeeded;
+
+  if (enableBackedgeSafepoints(F)) {
+    // Construct a pass manager to run the LoopPass backedge logic.  We
+    // need the pass manager to handle scheduling all the loop passes
+    // appropriately.  Doing this by hand is painful and just not worth messing
+    // with for the moment.
+    legacy::FunctionPassManager FPM(F.getParent());
+    bool CanAssumeCallSafepoints = enableCallSafepoints(F);
+    auto *PBS = new PlaceBackedgeSafepointsLegacyPass(CanAssumeCallSafepoints);
+    FPM.add(PBS);
+    FPM.run(F);
+
+    // We preserve dominance information when inserting the poll, otherwise
+    // we'd have to recalculate this on every insert
+    DT.recalculate(F);
+
+    auto &PollLocations = PBS->PollLocations;
+
+    auto OrderByBBName = [](Instruction *a, Instruction *b) {
+      return a->getParent()->getName() < b->getParent()->getName();
+    };
+    // We need the order of list to be stable so that naming ends up stable
+    // when we split edges.  This makes test cases much easier to write.
+    llvm::sort(PollLocations, OrderByBBName);
+
+    // We can sometimes end up with duplicate poll locations.  This happens if
+    // a single loop is visited more than once.   The fact this happens seems
+    // wrong, but it does happen for the split-backedge.ll test case.
+    PollLocations.erase(std::unique(PollLocations.begin(), PollLocations.end()),
+                        PollLocations.end());
+
+    // Insert a poll at each point the analysis pass identified
+    // The poll location must be the terminator of a loop latch block.
+    for (Instruction *Term : PollLocations) {
+      // We are inserting a poll, the function is modified
+      Modified = true;
+
+      if (SplitBackedge) {
+        // Split the backedge of the loop and insert the poll within that new
+        // basic block.  This creates a loop with two latches per original
+        // latch (which is non-ideal), but this appears to be easier to
+        // optimize in practice than inserting the poll immediately before the
+        // latch test.
+
+        // Since this is a latch, at least one of the successors must dominate
+        // it. Its possible that we have a) duplicate edges to the same header
+        // and b) edges to distinct loop headers.  We need to insert pools on
+        // each.
+        SetVector<BasicBlock *> Headers;
+        for (unsigned i = 0; i < Term->getNumSuccessors(); i++) {
+          BasicBlock *Succ = Term->getSuccessor(i);
+          if (DT.dominates(Succ, Term->getParent())) {
+            Headers.insert(Succ);
+          }
+        }
+        assert(!Headers.empty() && "poll location is not a loop latch?");
+
+        // The split loop structure here is so that we only need to recalculate
+        // the dominator tree once.  Alternatively, we could just keep it up to
+        // date and use a more natural merged loop.
+        SetVector<BasicBlock *> SplitBackedges;
+        for (BasicBlock *Header : Headers) {
+          BasicBlock *NewBB = SplitEdge(Term->getParent(), Header, &DT);
+          PollsNeeded.push_back(NewBB->getTerminator());
+          NumBackedgeSafepoints++;
+        }
+      } else {
+        // Split the latch block itself, right before the terminator.
+        PollsNeeded.push_back(Term);
+        NumBackedgeSafepoints++;
+      }
+    }
+  }
+
+  if (enableEntrySafepoints(F)) {
+    if (Instruction *Location = findLocationForEntrySafepoint(F, DT)) {
+      PollsNeeded.push_back(Location);
+      Modified = true;
+      NumEntrySafepoints++;
+    }
+    // TODO: else we should assert that there was, in fact, a policy choice to
+    // not insert a entry safepoint poll.
+  }
+
+  // Now that we've identified all the needed safepoint poll locations, insert
+  // safepoint polls themselves.
+  for (Instruction *PollLocation : PollsNeeded) {
+    std::vector<CallBase *> RuntimeCalls;
+    InsertSafepointPoll(PollLocation, RuntimeCalls, TLI);
+    llvm::append_range(ParsePointNeeded, RuntimeCalls);
+  }
+
+  return Modified;
+}
+
+PreservedAnalyses PlaceSafepointsPass::run(Function &F,
+                                           FunctionAnalysisManager &AM) {
+  auto &TLI = AM.getResult<TargetLibraryAnalysis>(F);
+
+  if (!runImpl(F, TLI))
+    return PreservedAnalyses::all();
+
+  // TODO: can we preserve more?
+  return PreservedAnalyses::none();
+}
 
 static bool needsStatepoint(CallBase *Call, const TargetLibraryInfo &TLI) {
   if (callsGCLeafFunction(Call, TLI))
     return false;
   if (auto *CI = dyn_cast<CallInst>(Call)) {
     if (CI->isInlineAsm())
       return false;
   }
static void scanInlinedCode(Instruction *Start, Instruction *End,
   scanOneBB(Start, End, Calls, Seen, Worklist);
   while (!Worklist.empty()) {
     BasicBlock *BB = Worklist.back();
     Worklist.pop_back();
     scanOneBB(&*BB->begin(), End, Calls, Seen, Worklist);
   }
 }
 
-bool PlaceBackedgeSafepointsImpl::runOnLoop(Loop *L) {
-  // Loop through all loop latches (branches controlling backedges).  We need
-  // to place a safepoint on every backedge (potentially).
-  // Note: In common usage, there will be only one edge due to LoopSimplify
-  // having run sometime earlier in the pipeline, but this code must be correct
-  // w.r.t. loops with multiple backedges.
-  BasicBlock *Header = L->getHeader();
-  SmallVector<BasicBlock*, 16> LoopLatches;
-  L->getLoopLatches(LoopLatches);
-  for (BasicBlock *Pred : LoopLatches) {
-    assert(L->contains(Pred));
-
-    // Make a policy decision about whether this loop needs a safepoint or
-    // not.  Note that this is about unburdening the optimizer in loops, not
-    // avoiding the runtime cost of the actual safepoint.
-    if (!AllBackedges) {
-      if (mustBeFiniteCountedLoop(L, SE, Pred)) {
-        LLVM_DEBUG(dbgs() << "skipping safepoint placement in finite loop\n");
-        FiniteExecution++;
-        continue;
-      }
-      if (CallSafepointsEnabled &&
-          containsUnconditionalCallSafepoint(L, Header, Pred, *DT, *TLI)) {
-        // Note: This is only semantically legal since we won't do any further
-        // IPO or inlining before the actual call insertion..  If we hadn't, we
-        // might latter loose this call safepoint.
-        LLVM_DEBUG(
-            dbgs()
-            << "skipping safepoint placement due to unconditional call\n");
-        CallInLoop++;
-        continue;
-      }
-    }
-
-    // TODO: We can create an inner loop which runs a finite number of
-    // iterations with an outer loop which contains a safepoint.  This would
-    // not help runtime performance that much, but it might help our ability to
-    // optimize the inner loop.
-
-    // Safepoint insertion would involve creating a new basic block (as the
-    // target of the current backedge) which does the safepoint (of all live
-    // variables) and branches to the true header
-    Instruction *Term = Pred->getTerminator();
-
-    LLVM_DEBUG(dbgs() << "[LSP] terminator instruction: " << *Term);
-
-    PollLocations.push_back(Term);
-  }
-
-  return false;
-}
-
 /// Returns true if an entry safepoint is not required before this callsite in
 /// the caller function.
 static bool doesNotRequireEntrySafepointBefore(CallBase *Call) {
   if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Call)) {
     switch (II->getIntrinsicID()) {
     case Intrinsic::experimental_gc_statepoint:
     case Intrinsic::experimental_patchpoint_void:
     case Intrinsic::experimental_patchpoint_i64:

 }
 
 // TODO: These should become properties of the GCStrategy, possibly with
 // command line overrides.
 static bool enableEntrySafepoints(Function &F) { return !NoEntry; }
 static bool enableBackedgeSafepoints(Function &F) { return !NoBackedge; }
 static bool enableCallSafepoints(Function &F) { return !NoCall; }
 
-bool PlaceSafepoints::runOnFunction(Function &F) {
-  if (F.isDeclaration() || F.empty()) {
-    // This is a declaration, nothing to do.  Must exit early to avoid crash in
-    // dom tree calculation
-    return false;
-  }
-
-  if (isGCSafepointPoll(F)) {
-    // Given we're inlining this inside of safepoint poll insertion, this
-    // doesn't make any sense.  Note that we do make any contained calls
-    // parseable after we inline a poll.
-    return false;
-  }
-
-  if (!shouldRewriteFunction(F))
-    return false;
-
-  const TargetLibraryInfo &TLI =
-      getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F);
-
-  bool Modified = false;
-
-  // In various bits below, we rely on the fact that uses are reachable from
-  // defs.  When there are basic blocks unreachable from the entry, dominance
-  // and reachablity queries return non-sensical results.  Thus, we preprocess
-  // the function to ensure these properties hold.
-  Modified |= removeUnreachableBlocks(F);
-
-  // STEP 1 - Insert the safepoint polling locations.  We do not need to
-  // actually insert parse points yet.  That will be done for all polls and
-  // calls in a single pass.
-
-  DominatorTree DT;
-  DT.recalculate(F);
-
-  SmallVector<Instruction *, 16> PollsNeeded;
-  std::vector<CallBase *> ParsePointNeeded;
-
-  if (enableBackedgeSafepoints(F)) {
-    // Construct a pass manager to run the LoopPass backedge logic.  We
-    // need the pass manager to handle scheduling all the loop passes
-    // appropriately.  Doing this by hand is painful and just not worth messing
-    // with for the moment.
-    legacy::FunctionPassManager FPM(F.getParent());
-    bool CanAssumeCallSafepoints = enableCallSafepoints(F);
-    auto *PBS = new PlaceBackedgeSafepointsImpl(CanAssumeCallSafepoints);
-    FPM.add(PBS);
-    FPM.run(F);
-
-    // We preserve dominance information when inserting the poll, otherwise
-    // we'd have to recalculate this on every insert
-    DT.recalculate(F);
-
-    auto &PollLocations = PBS->PollLocations;
-
-    auto OrderByBBName = [](Instruction *a, Instruction *b) {
-      return a->getParent()->getName() < b->getParent()->getName();
-    };
-    // We need the order of list to be stable so that naming ends up stable
-    // when we split edges.  This makes test cases much easier to write.
-    llvm::sort(PollLocations, OrderByBBName);
-
-    // We can sometimes end up with duplicate poll locations.  This happens if
-    // a single loop is visited more than once.   The fact this happens seems
-    // wrong, but it does happen for the split-backedge.ll test case.
-    PollLocations.erase(std::unique(PollLocations.begin(),
-                                    PollLocations.end()),
-                        PollLocations.end());
-
-    // Insert a poll at each point the analysis pass identified
-    // The poll location must be the terminator of a loop latch block.
-    for (Instruction *Term : PollLocations) {
-      // We are inserting a poll, the function is modified
-      Modified = true;
-
-      if (SplitBackedge) {
-        // Split the backedge of the loop and insert the poll within that new
-        // basic block.  This creates a loop with two latches per original
-        // latch (which is non-ideal), but this appears to be easier to
-        // optimize in practice than inserting the poll immediately before the
-        // latch test.
-
-        // Since this is a latch, at least one of the successors must dominate
-        // it. Its possible that we have a) duplicate edges to the same header
-        // and b) edges to distinct loop headers.  We need to insert pools on
-        // each.
-        SetVector<BasicBlock *> Headers;
-        for (unsigned i = 0; i < Term->getNumSuccessors(); i++) {
-          BasicBlock *Succ = Term->getSuccessor(i);
-          if (DT.dominates(Succ, Term->getParent())) {
-            Headers.insert(Succ);
-          }
-        }
-        assert(!Headers.empty() && "poll location is not a loop latch?");
-
-        // The split loop structure here is so that we only need to recalculate
-        // the dominator tree once.  Alternatively, we could just keep it up to
-        // date and use a more natural merged loop.
-        SetVector<BasicBlock *> SplitBackedges;
-        for (BasicBlock *Header : Headers) {
-          BasicBlock *NewBB = SplitEdge(Term->getParent(), Header, &DT);
-          PollsNeeded.push_back(NewBB->getTerminator());
-          NumBackedgeSafepoints++;
-        }
-      } else {
-        // Split the latch block itself, right before the terminator.
-        PollsNeeded.push_back(Term);
-        NumBackedgeSafepoints++;
-      }
-    }
-  }
-
-  if (enableEntrySafepoints(F)) {
-    if (Instruction *Location = findLocationForEntrySafepoint(F, DT)) {
-      PollsNeeded.push_back(Location);
-      Modified = true;
-      NumEntrySafepoints++;
-    }
-    // TODO: else we should assert that there was, in fact, a policy choice to
-    // not insert a entry safepoint poll.
-  }
-
-  // Now that we've identified all the needed safepoint poll locations, insert
-  // safepoint polls themselves.
-  for (Instruction *PollLocation : PollsNeeded) {
-    std::vector<CallBase *> RuntimeCalls;
-    InsertSafepointPoll(PollLocation, RuntimeCalls, TLI);
-    llvm::append_range(ParsePointNeeded, RuntimeCalls);
-  }
-
-  return Modified;
-}
-
-char PlaceBackedgeSafepointsImpl::ID = 0;
-char PlaceSafepoints::ID = 0;
-
-FunctionPass *llvm::createPlaceSafepointsPass() {
-  return new PlaceSafepoints();
-}
-
-INITIALIZE_PASS_BEGIN(PlaceBackedgeSafepointsImpl,
-                      "place-backedge-safepoints-impl",
-                      "Place Backedge Safepoints", false, false)
-INITIALIZE_PASS_DEPENDENCY(ScalarEvolutionWrapperPass)
-INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
-INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
-INITIALIZE_PASS_END(PlaceBackedgeSafepointsImpl,
-                    "place-backedge-safepoints-impl",
-                    "Place Backedge Safepoints", false, false)
-
-INITIALIZE_PASS_BEGIN(PlaceSafepoints, "place-safepoints", "Place Safepoints",
-                      false, false)
-INITIALIZE_PASS_END(PlaceSafepoints, "place-safepoints", "Place Safepoints",
-                    false, false)
-
+// Insert a safepoint poll immediately before the given instruction.  Does
+// not handle the parsability of state at the runtime call, that's the
+// callers job.
 static void
 InsertSafepointPoll(Instruction *InsertBefore,
                     std::vector<CallBase *> &ParsePointsNeeded /*rval*/,
                     const TargetLibraryInfo &TLI) {
   BasicBlock *OrigBB = InsertBefore->getParent();
   Module *M = InsertBefore->getModule();
   assert(M && "must be part of a module");
 

llvm/lib/Transforms/Scalar/Scalar.cpp

void llvm::initializeScalarOpts(PassRegistry &Registry) {
   initializeStructurizeCFGLegacyPassPass(Registry);
   initializeSimpleLoopUnswitchLegacyPassPass(Registry);
   initializeSinkingLegacyPassPass(Registry);
   initializeTailCallElimPass(Registry);
   initializeTLSVariableHoistLegacyPassPass(Registry);
   initializeSeparateConstOffsetFromGEPLegacyPassPass(Registry);
   initializeSpeculativeExecutionLegacyPassPass(Registry);
   initializeStraightLineStrengthReduceLegacyPassPass(Registry);
-  initializePlaceBackedgeSafepointsImplPass(Registry);
-  initializePlaceSafepointsPass(Registry);
+  initializePlaceBackedgeSafepointsLegacyPassPass(Registry);
+  initializePlaceSafepointsLegacyPassPass(Registry);
   initializeFloat2IntLegacyPassPass(Registry);
   initializeLoopSimplifyCFGLegacyPassPass(Registry);
 }
 
 void LLVMAddLoopSimplifyCFGPass(LLVMPassManagerRef PM) {
   unwrap(PM)->add(createLoopSimplifyCFGPass());
 }
 

llvm/test/Transforms/PlaceSafepoints/basic.ll

-; RUN: opt < %s -S -place-safepoints -enable-new-pm=0 | FileCheck %s
-
+; RUN: opt < %s -S -passes=place-safepoints | FileCheck %s
 
 ; Do we insert a simple entry safepoint?
 define void @test_entry() gc "statepoint-example" {
 ; CHECK-LABEL: @test_entry
 entry:
 ; CHECK-LABEL: entry
 ; CHECK: call void @do_safepoint
   ret void

llvm/test/Transforms/PlaceSafepoints/call-in-loop.ll

 ; If there's a call in the loop which dominates the backedge, we 
 ; don't need a safepoint poll (since the callee must contain a 
 ; poll test).
-;; RUN: opt < %s -place-safepoints -S -enable-new-pm=0 | FileCheck %s
+;; RUN: opt < %s -passes=place-safepoints -S | FileCheck %s
 
 declare void @foo()
 
 define void @test1() gc "statepoint-example" {
 ; CHECK-LABEL: test1
 
 entry:
 ; CHECK-LABEL: entry

llvm/test/Transforms/PlaceSafepoints/finite-loops.ll

 ; Tests to ensure that we are not placing backedge safepoints in
 ; loops which are clearly finite.
-;; RUN: opt < %s -place-safepoints -spp-counted-loop-trip-width=32 -S -enable-new-pm=0 | FileCheck %s
-;; RUN: opt < %s -place-safepoints -spp-counted-loop-trip-width=64 -S -enable-new-pm=0 | FileCheck %s -check-prefix=COUNTED-64
+;; RUN: opt < %s -passes=place-safepoints -spp-counted-loop-trip-width=32 -S | FileCheck %s
+;; RUN: opt < %s -passes=place-safepoints -spp-counted-loop-trip-width=64 -S | FileCheck %s -check-prefix=COUNTED-64
 
 
 ; A simple counted loop with trivially known range
 define void @test1(i32) gc "statepoint-example" {
 ; CHECK-LABEL: test1
 ; CHECK-LABEL: entry
 ; CHECK: call void @do_safepoint
 ; CHECK-LABEL: loop

llvm/test/Transforms/PlaceSafepoints/libcall.ll

-; RUN: opt -S -place-safepoints < %s -enable-new-pm=0 | FileCheck %s
+; RUN: opt -S -passes=place-safepoints < %s | FileCheck %s
 
 ; Libcalls will not contain a safepoint poll, so check that we insert
 ; a safepoint in a loop containing a libcall.
 declare double @ldexp(double %x, i32 %n) nounwind readnone
 define double @test_libcall(double %x) gc "statepoint-example" {
 ; CHECK-LABEL: test_libcall
 
 entry:

llvm/test/Transforms/PlaceSafepoints/memset.ll

-; RUN: opt < %s -S -place-safepoints -enable-new-pm=0 | FileCheck %s
+; RUN: opt < %s -S -passes=place-safepoints | FileCheck %s
 
 define void @test(i32, ptr addrspace(1) %ptr) gc "statepoint-example" {
 ; CHECK-LABEL: @test
 ; CHECK-NEXT: llvm.memset
 ; CHECK: do_safepoint
 ; CHECK: @foo
   call void @llvm.memset.p1.i64(ptr addrspace(1) align 8 %ptr, i8 0, i64 24, i1 false)
   call void @foo()

llvm/test/Transforms/PlaceSafepoints/no-statepoints.ll

-; RUN: opt -S -place-safepoints < %s -enable-new-pm=0 | FileCheck %s
+; RUN: opt -S -passes=place-safepoints < %s | FileCheck %s
 
 declare void @callee()
 
 define void @test() gc "statepoint-example" {
 ; CHECK-LABEL: test(
 entry:
 ; CHECK: entry:
 ; CHECK: call void @do_safepoint()

llvm/test/Transforms/PlaceSafepoints/split-backedge.ll

 ;; A very basic test to make sure that splitting the backedge keeps working
-;; RUN: opt < %s -place-safepoints -spp-split-backedge=1 -S -enable-new-pm=0 | FileCheck %s
+;; RUN: opt < %s -passes=place-safepoints -spp-split-backedge=1 -S | FileCheck %s
 
 define void @test(i32, i1 %cond) gc "statepoint-example" {
 ; CHECK-LABEL: @test
 ; CHECK-LABEL: loop.loop_crit_edge
 ; CHECK: call void @do_safepoint
 ; CHECK-NEXT: br label %loop
 entry:
   br label %loop

llvm/test/Transforms/PlaceSafepoints/statepoint-coreclr.ll

-; RUN: opt < %s -S -place-safepoints -enable-new-pm=0 | FileCheck %s
+; RUN: opt < %s -S -passes=place-safepoints | FileCheck %s
 
 ; Basic test to make sure that safepoints are placed
 ; for CoreCLR GC
 
 declare void @foo()
 
 define void @test_simple_call() gc "coreclr" {
 ; CHECK-LABEL: test_simple_call

llvm/test/Transforms/PlaceSafepoints/statepoint-frameescape.ll

-; RUN: opt < %s -S -place-safepoints -enable-new-pm=0 | FileCheck %s
+; RUN: opt < %s -S -passes=place-safepoints | FileCheck %s
 
 declare void @llvm.localescape(...)
 
 ; Do we insert the entry safepoint after the localescape intrinsic?
 define void @parent() gc "statepoint-example" {
 ; CHECK-LABEL: @parent
 entry:
 ; CHECK-LABEL: entry