[LoopInfo] Make LoopBase and Loop destructors non-public

[sanjoy]

Summary

See comment for why I think this is a good idea.

This change also:

• Removes an SCEV test case. The SCEV test was not testing anything useful (most of it was #if 0 ed out) and it would need to be updated to deal with a private ~Loop::Loop.
• Updates the loop pass manager test case to deal with a private ~Loop::Loop.
• Renames markAsRemoved to markAsErased to contrast with removeLoop, via the usual remove vs. erase idiom we already have for instructions and basic blocks.

[chandlerc]

LGTM, nits below.

include/llvm/Analysis/LoopInfo.h

343–349

Rather than using llvm:: prefixes, I'd just use markdown around the name, for example: `Loop` and `LoopInfo`.

lib/Analysis/LoopInfo.cpp

625

This should really be erase rather than markAsErased... But happy for that to be a later change.

[sanjoy]

lib/Analysis/LoopInfo.cpp

625

I'll do that renaming once the bump ptr allocator change has landed.

include/llvm/Analysis/LoopInfo.h

class LoopBase {
   bool IsInvalid = false;
 
   LoopBase(const LoopBase<BlockT, LoopT> &) = delete;
   const LoopBase<BlockT, LoopT>&
     operator=(const LoopBase<BlockT, LoopT> &) = delete;
 public:
   /// This creates an empty loop.
   LoopBase() : ParentLoop(nullptr) {}
-  ~LoopBase() {
-    for (size_t i = 0, e = SubLoops.size(); i != e; ++i)
-      delete SubLoops[i];
-  }
 
   /// Return the nesting level of this loop.  An outer-most loop has depth 1,
   /// for consistency with loop depth values used for basic blocks, where depth
   /// 0 is used for blocks not inside any loops.
   unsigned getLoopDepth() const {
     unsigned D = 1;
     for (const LoopT *CurLoop = ParentLoop; CurLoop;
          CurLoop = CurLoop->ParentLoop)
public:
   void print(raw_ostream &OS, unsigned Depth = 0, bool Verbose = false) const;
 
 protected:
   friend class LoopInfoBase<BlockT, LoopT>;
   explicit LoopBase(BlockT *BB) : ParentLoop(nullptr) {
     Blocks.push_back(BB);
     DenseBlockSet.insert(BB);
   }
+
+  // Since loop passes like SCEV are allowed to key analysis results off of
+  // llvm::Loop pointers, we cannot re-use pointers within a loop pass manager.
+  // This means loop passes should not be `delete` ing llvm::Loop objects
+  // directly (and risk a later llvm::Loop allocation re-using the address of a
+  // previous one) but should be using LoopInfo::markAsRemoved, which keeps
+  // around the llvm::Loop pointer till the end of the lifetime of the
+  // llvm::LoopInfo object.
+  //
+  // To make it easier to follow this rule, we mark the destructor as
+  // non-public.
+  ~LoopBase() {
+    for (auto *SubLoop : SubLoops)
+      delete SubLoop;
+  }
 };
 
 template<class BlockT, class LoopT>
 raw_ostream& operator<<(raw_ostream &OS, const LoopBase<BlockT, LoopT> &Loop) {
   Loop.print(OS);
   return OS;
 }
 
StringRef getName() const {
     if (BasicBlock *Header = getHeader())
       if (Header->hasName())
         return Header->getName();
     return "<unnamed loop>";
   }
 
 private:
   friend class LoopInfoBase<BasicBlock, Loop>;
+  friend class LoopBase<BasicBlock, Loop>;
   explicit Loop(BasicBlock *BB) : LoopBase<BasicBlock, Loop>(BB) {}
+  ~Loop() = default;
 };
 
 //===----------------------------------------------------------------------===//
 /// This class builds and contains all of the top-level loop
 /// structures in the specified function.
 ///
 
 template<class BlockT, class LoopT>
bool invalidate(Function &F, const PreservedAnalyses &PA,
                   FunctionAnalysisManager::Invalidator &);
 
   // Most of the public interface is provided via LoopInfoBase.
 
   /// Update LoopInfo after removing the last backedge from a loop. This updates
   /// the loop forest and parent loops for each block so that \c L is no longer
   /// referenced, but does not actually delete \c L immediately. The pointer
   /// will remain valid until this LoopInfo's memory is released.
-  void markAsRemoved(Loop *L);
+  void markAsErased(Loop *L);
 
   /// Returns true if replacing From with To everywhere is guaranteed to
   /// preserve LCSSA form.
   bool replacementPreservesLCSSAForm(Instruction *From, Value *To) {
     // Preserving LCSSA form is only problematic if the replacing value is an
     // instruction.
     Instruction *I = dyn_cast<Instruction>(To);
     if (!I) return true;

lib/Analysis/LoopInfo.cpp

bool LoopInfo::invalidate(Function &F, const PreservedAnalyses &PA,
                           FunctionAnalysisManager::Invalidator &) {
   // Check whether the analysis, all analyses on functions, or the function's
   // CFG have been preserved.
   auto PAC = PA.getChecker<LoopAnalysis>();
   return !(PAC.preserved() || PAC.preservedSet<AllAnalysesOn<Function>>() ||
            PAC.preservedSet<CFGAnalyses>());
 }
 
-void LoopInfo::markAsRemoved(Loop *Unloop) {
-  assert(!Unloop->isInvalid() && "Loop has already been removed");
+void LoopInfo::markAsErased(Loop *Unloop) {
+  assert(!Unloop->isInvalid() && "Loop has already been erased!");
   Unloop->invalidate();
   RemovedLoops.push_back(Unloop);
 
   // First handle the special case of no parent loop to simplify the algorithm.
   if (!Unloop->getParentLoop()) {
     // Since BBLoop had no parent, Unloop blocks are no longer in a loop.
     for (Loop::block_iterator I = Unloop->block_begin(),
                               E = Unloop->block_end();

lib/Transforms/IPO/LoopExtractor.cpp

bool LoopExtractor::runOnLoop(Loop *L, LPPassManager &) {
   if (ShouldExtractLoop) {
     if (NumLoops == 0) return Changed;
     --NumLoops;
     CodeExtractor Extractor(DT, *L);
     if (Extractor.extractCodeRegion() != nullptr) {
       Changed = true;
       // After extraction, the loop is replaced by a function call, so
       // we shouldn't try to run any more loop passes on it.
-      LI.markAsRemoved(L);
+      LI.markAsErased(L);
     }
     ++NumExtracted;
   }
 
   return Changed;
 }
 
 // createSingleLoopExtractorPass - This pass extracts one natural loop from the

lib/Transforms/Scalar/LoopDeletion.cpp

static void deleteDeadLoop(Loop *L, DominatorTree &DT, ScalarEvolution &SE,
   // otherwise our loop iterators won't work.
 
   SmallPtrSet<BasicBlock *, 8> blocks;
   blocks.insert(L->block_begin(), L->block_end());
   for (BasicBlock *BB : blocks)
     LI.removeBlock(BB);
 
   // The last step is to update LoopInfo now that we've eliminated this loop.
-  LI.markAsRemoved(L);
+  LI.markAsErased(L);
 }
 
 PreservedAnalyses LoopDeletionPass::run(Loop &L, LoopAnalysisManager &AM,
                                         LoopStandardAnalysisResults &AR,
                                         LPMUpdater &Updater) {
 
   DEBUG(dbgs() << "Analyzing Loop for deletion: ");
   DEBUG(L.dump());

lib/Transforms/Utils/LoopUnroll.cpp

bool llvm::UnrollLoop(Loop *L, unsigned Count, unsigned TripCount, bool Force,
   // appropriate.
 
   NumCompletelyUnrolled += CompletelyUnroll;
   ++NumUnrolled;
 
   Loop *OuterL = L->getParentLoop();
   // Update LoopInfo if the loop is completely removed.
   if (CompletelyUnroll)
-    LI->markAsRemoved(L);
+    LI->markAsErased(L);
 
   // After complete unrolling most of the blocks should be contained in OuterL.
   // However, some of them might happen to be out of OuterL (e.g. if they
   // precede a loop exit). In this case we might need to insert PHI nodes in
   // order to preserve LCSSA form.
   // We don't need to check this if we already know that we need to fix LCSSA
   // form.
   // TODO: For now we just recompute LCSSA for the outer loop in this case, but
   // it should be possible to fix it in-place.
   if (PreserveLCSSA && OuterL && CompletelyUnroll && !NeedToFixLCSSA)
     NeedToFixLCSSA |= ::needToInsertPhisForLCSSA(OuterL, UnrolledLoopBlocks, LI);
 
   // If we have a pass and a DominatorTree we should re-simplify impacted loops
   // to ensure subsequent analyses can rely on this form. We want to simplify
   // at least one layer outside of the loop that was unrolled so that any
   // changes to the parent loop exposed by the unrolling are considered.
   if (DT) {
     if (OuterL) {
       // OuterL includes all loops for which we can break loop-simplify, so
       // it's sufficient to simplify only it (it'll recursively simplify inner
       // loops too).
       if (NeedToFixLCSSA) {
         // LCSSA must be performed on the outermost affected loop. The unrolled
         // loop's last loop latch is guaranteed to be in the outermost loop
-        // after LoopInfo's been updated by markAsRemoved.
+        // after LoopInfo's been updated by markAsErased.
         Loop *LatchLoop = LI->getLoopFor(Latches.back());
         Loop *FixLCSSALoop = OuterL;
         if (!FixLCSSALoop->contains(LatchLoop))
           while (FixLCSSALoop->getParentLoop() != LatchLoop)
             FixLCSSALoop = FixLCSSALoop->getParentLoop();
 
         formLCSSARecursively(*FixLCSSALoop, *DT, LI, SE);
       } else if (PreserveLCSSA) {

unittests/Analysis/ScalarEvolutionTest.cpp

TEST_F(ScalarEvolutionsTest, SCEVUnknownRAUW) {
   V1->replaceAllUsesWith(V0);
 
   // After the RAUWs, these should all be pointing to V0.
   EXPECT_EQ(cast<SCEVUnknown>(M0->getOperand(1))->getValue(), V0);
   EXPECT_EQ(cast<SCEVUnknown>(M1->getOperand(1))->getValue(), V0);
   EXPECT_EQ(cast<SCEVUnknown>(M2->getOperand(1))->getValue(), V0);
 }
 
-TEST_F(ScalarEvolutionsTest, SCEVMultiplyAddRecs) {
-  Type *Ty = Type::getInt32Ty(Context);
-  SmallVector<Type *, 10> Types;
-  Types.append(10, Ty);
-  FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context), Types, false);
-  Function *F = cast<Function>(M.getOrInsertFunction("f", FTy));
-  BasicBlock *BB = BasicBlock::Create(Context, "entry", F);
-  ReturnInst::Create(Context, nullptr, BB);
-
-  ScalarEvolution SE = buildSE(*F);
-
-  // It's possible to produce an empty loop through the default constructor,
-  // but you can't add any blocks to it without a LoopInfo pass.
-  Loop L;
-  const_cast<std::vector<BasicBlock*>&>(L.getBlocks()).push_back(BB);
-
-  Function::arg_iterator AI = F->arg_begin();
-  SmallVector<const SCEV *, 5> A;
-  A.push_back(SE.getSCEV(&*AI++));
-  A.push_back(SE.getSCEV(&*AI++));
-  A.push_back(SE.getSCEV(&*AI++));
-  A.push_back(SE.getSCEV(&*AI++));
-  A.push_back(SE.getSCEV(&*AI++));
-  const SCEV *A_rec = SE.getAddRecExpr(A, &L, SCEV::FlagAnyWrap);
-
-  SmallVector<const SCEV *, 5> B;
-  B.push_back(SE.getSCEV(&*AI++));
-  B.push_back(SE.getSCEV(&*AI++));
-  B.push_back(SE.getSCEV(&*AI++));
-  B.push_back(SE.getSCEV(&*AI++));
-  B.push_back(SE.getSCEV(&*AI++));
-  const SCEV *B_rec = SE.getAddRecExpr(B, &L, SCEV::FlagAnyWrap);
-
-  /* Spot check that we perform this transformation:
-     {A0,+,A1,+,A2,+,A3,+,A4} * {B0,+,B1,+,B2,+,B3,+,B4} =
-     {A0*B0,+,
-      A1*B0 + A0*B1 + A1*B1,+,
-      A2*B0 + 2A1*B1 + A0*B2 + 2A2*B1 + 2A1*B2 + A2*B2,+,
-      A3*B0 + 3A2*B1 + 3A1*B2 + A0*B3 + 3A3*B1 + 6A2*B2 + 3A1*B3 + 3A3*B2 +
-        3A2*B3 + A3*B3,+,
-      A4*B0 + 4A3*B1 + 6A2*B2 + 4A1*B3 + A0*B4 + 4A4*B1 + 12A3*B2 + 12A2*B3 +
-        4A1*B4 + 6A4*B2 + 12A3*B3 + 6A2*B4 + 4A4*B3 + 4A3*B4 + A4*B4,+,
-      5A4*B1 + 10A3*B2 + 10A2*B3 + 5A1*B4 + 20A4*B2 + 30A3*B3 + 20A2*B4 +
-        30A4*B3 + 30A3*B4 + 20A4*B4,+,
-      15A4*B2 + 20A3*B3 + 15A2*B4 + 60A4*B3 + 60A3*B4 + 90A4*B4,+,
-      35A4*B3 + 35A3*B4 + 140A4*B4,+,
-      70A4*B4}
-  */
-
-  const SCEVAddRecExpr *Product =
-      dyn_cast<SCEVAddRecExpr>(SE.getMulExpr(A_rec, B_rec));
-  ASSERT_TRUE(Product);
-  ASSERT_EQ(Product->getNumOperands(), 9u);
-
-  SmallVector<const SCEV *, 16> Sum;
-  Sum.push_back(SE.getMulExpr(A[0], B[0]));
-  EXPECT_EQ(Product->getOperand(0), SE.getAddExpr(Sum));
-  Sum.clear();
-
-  // SCEV produces different an equal but different expression for these.
-  // Re-enable when PR11052 is fixed.
-#if 0
-  Sum.push_back(SE.getMulExpr(A[1], B[0]));
-  Sum.push_back(SE.getMulExpr(A[0], B[1]));
-  Sum.push_back(SE.getMulExpr(A[1], B[1]));
-  EXPECT_EQ(Product->getOperand(1), SE.getAddExpr(Sum));
-  Sum.clear();
-
-  Sum.push_back(SE.getMulExpr(A[2], B[0]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[1], B[1]));
-  Sum.push_back(SE.getMulExpr(A[0], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[2], B[1]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[1], B[2]));
-  Sum.push_back(SE.getMulExpr(A[2], B[2]));
-  EXPECT_EQ(Product->getOperand(2), SE.getAddExpr(Sum));
-  Sum.clear();
-
-  Sum.push_back(SE.getMulExpr(A[3], B[0]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[2], B[1]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[1], B[2]));
-  Sum.push_back(SE.getMulExpr(A[0], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[3], B[1]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[1], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[3], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[2], B[3]));
-  Sum.push_back(SE.getMulExpr(A[3], B[3]));
-  EXPECT_EQ(Product->getOperand(3), SE.getAddExpr(Sum));
-  Sum.clear();
-
-  Sum.push_back(SE.getMulExpr(A[4], B[0]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[3], B[1]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[1], B[3]));
-  Sum.push_back(SE.getMulExpr(A[0], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[4], B[1]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[3], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[2], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[1], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[4], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[3], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[4], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[3], B[4]));
-  Sum.push_back(SE.getMulExpr(A[4], B[4]));
-  EXPECT_EQ(Product->getOperand(4), SE.getAddExpr(Sum));
-  Sum.clear();
-
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 5), A[4], B[1]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 10), A[3], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 10), A[2], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 5), A[1], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[4], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[3], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[2], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[4], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[3], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[4], B[4]));
-  EXPECT_EQ(Product->getOperand(5), SE.getAddExpr(Sum));
-  Sum.clear();
-
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 15), A[4], B[2]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[3], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 15), A[2], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 60), A[4], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 60), A[3], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 90), A[4], B[4]));
-  EXPECT_EQ(Product->getOperand(6), SE.getAddExpr(Sum));
-  Sum.clear();
-
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 35), A[4], B[3]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 35), A[3], B[4]));
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 140), A[4], B[4]));
-  EXPECT_EQ(Product->getOperand(7), SE.getAddExpr(Sum));
-  Sum.clear();
-#endif
-
-  Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 70), A[4], B[4]));
-  EXPECT_EQ(Product->getOperand(8), SE.getAddExpr(Sum));
-}
-
 TEST_F(ScalarEvolutionsTest, SimplifiedPHI) {
   FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context),
                                               std::vector<Type *>(), false);
   Function *F = cast<Function>(M.getOrInsertFunction("f", FTy));
   BasicBlock *EntryBB = BasicBlock::Create(Context, "entry", F);
   BasicBlock *LoopBB = BasicBlock::Create(Context, "loop", F);
   BasicBlock *ExitBB = BasicBlock::Create(Context, "exit", F);
   BranchInst::Create(LoopBB, EntryBB);

unittests/Transforms/Scalar/LoopPassManagerTest.cpp

TEST_F(LoopPassManagerTest, LoopDeletion) {
   BasicBlock &EndBB = *BBI++;
   ASSERT_THAT(EndBB, HasName("end"));
   ASSERT_THAT(BBI, F.end());
 
   // Helper to do the actual deletion of a loop. We directly encode this here
   // to isolate ourselves from the rest of LLVM and for simplicity. Here we can
   // egregiously cheat based on knowledge of the test case. For example, we
   // have no PHI nodes and there is always a single i-dom.
-  auto RemoveLoop = [](Loop &L, BasicBlock &IDomBB,
-                             LoopStandardAnalysisResults &AR,
-                             LPMUpdater &Updater) {
+  auto EraseLoop = [](Loop &L, BasicBlock &IDomBB,
+                      LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
     assert(L.empty() && "Can only delete leaf loops with this routine!");
     SmallVector<BasicBlock *, 4> LoopBBs(L.block_begin(), L.block_end());
     Updater.markLoopAsDeleted(L);
     IDomBB.getTerminator()->replaceUsesOfWith(L.getHeader(),
                                               L.getUniqueExitBlock());
     for (BasicBlock *LoopBB : LoopBBs) {
       SmallVector<DomTreeNode *, 4> ChildNodes(AR.DT[LoopBB]->begin(),
                                                AR.DT[LoopBB]->end());
       for (DomTreeNode *ChildNode : ChildNodes)
         AR.DT.changeImmediateDominator(ChildNode, AR.DT[&IDomBB]);
       AR.DT.eraseNode(LoopBB);
       AR.LI.removeBlock(LoopBB);
       LoopBB->dropAllReferences();
     }
     for (BasicBlock *LoopBB : LoopBBs)
       LoopBB->eraseFromParent();
 
-    if (Loop *ParentL = L.getParentLoop())
-      return ParentL->removeChildLoop(find(*ParentL, &L));
-
-    return AR.LI.removeLoop(find(AR.LI, &L));
+    AR.LI.markAsErased(&L);
   };
 
   // Build up the pass managers.
   ModulePassManager MPM(true);
   FunctionPassManager FPM(true);
   // We run several loop pass pipelines across the loop nest, but they all take
   // the same form of three mock pass runs in a loop pipeline followed by
   // domtree and loop verification. We use a lambda to stamp this out each
EXPECT_CALL(MLPHandle, run(HasName("loop.0.1"), _, _, _))
       .WillOnce(Invoke(getLoopAnalysisResult));
   EXPECT_CALL(MLAHandle, run(HasName("loop.0.1"), _, _));
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.1"), _, _, _))
       .WillOnce(
           Invoke([&](Loop &L, LoopAnalysisManager &AM,
                      LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
             Loop *ParentL = L.getParentLoop();
             AR.SE.forgetLoop(&L);
-            delete RemoveLoop(L, Loop01PHBB, AR, Updater);
+            EraseLoop(L, Loop01PHBB, AR, Updater);
             ParentL->verifyLoop();
             return PreservedAnalyses::all();
           }));
 
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.2.0"), _, _, _))
       .WillOnce(Invoke(getLoopAnalysisResult));
   EXPECT_CALL(MLAHandle, run(HasName("loop.0.2.0"), _, _));
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.2.0"), _, _, _))
TEST_F(LoopPassManagerTest, LoopDeletion) {
   EXPECT_CALL(MLPHandle, run(HasName("loop.0"), _, _, _))
       .WillOnce(Invoke(getLoopAnalysisResult));
   EXPECT_CALL(MLAHandle, run(HasName("loop.0"), _, _));
   EXPECT_CALL(MLPHandle, run(HasName("loop.0"), _, _, _))
       .Times(2)
       .WillRepeatedly(Invoke(getLoopAnalysisResult));
 
   // Run the loop pipeline again. This time we delete the last loop, which
-  // contains a nested loop within it, and we reuse its inner loop object to
-  // insert a new loop into the nest. This makes sure that we don't reuse
-  // cached analysis results for loop objects when removed just because their
-  // pointers match, and that we can handle nested loop deletion.
+  // contains a nested loop within it and insert a new loop into the nest. This
+  // makes sure we can handle nested loop deletion.
   AddLoopPipelineAndVerificationPasses();
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _))
       .Times(3)
       .WillRepeatedly(Invoke(getLoopAnalysisResult));
 
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.2.0"), _, _, _))
       .Times(3)
       .WillRepeatedly(Invoke(getLoopAnalysisResult));
 
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.2"), _, _, _))
       .WillOnce(Invoke(getLoopAnalysisResult));
   BasicBlock *NewLoop03PHBB;
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.2"), _, _, _))
       .WillOnce(
           Invoke([&](Loop &L, LoopAnalysisManager &AM,
                      LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
-            // Remove the inner loop first but retain it to reuse later.
             AR.SE.forgetLoop(*L.begin());
-            auto *OldL = RemoveLoop(**L.begin(), Loop020PHBB, AR, Updater);
+            EraseLoop(**L.begin(), Loop020PHBB, AR, Updater);
 
             auto *ParentL = L.getParentLoop();
             AR.SE.forgetLoop(&L);
-            delete RemoveLoop(L, Loop02PHBB, AR, Updater);
+            EraseLoop(L, Loop02PHBB, AR, Updater);
 
-            // Now insert a new sibling loop, reusing a loop pointer.
-            ParentL->addChildLoop(OldL);
+            // Now insert a new sibling loop.
+            auto *NewSibling = new Loop;
+            ParentL->addChildLoop(NewSibling);
             NewLoop03PHBB =
                 BasicBlock::Create(Context, "loop.0.3.ph", &F, &Loop0LatchBB);
             auto *NewLoop03BB =
                 BasicBlock::Create(Context, "loop.0.3", &F, &Loop0LatchBB);
             BranchInst::Create(NewLoop03BB, NewLoop03PHBB);
             auto *Cond = new LoadInst(&Ptr, "cond.0.3", /*isVolatile*/ true,
                                       NewLoop03BB);
             BranchInst::Create(&Loop0LatchBB, NewLoop03BB, Cond, NewLoop03BB);
             Loop02PHBB.getTerminator()->replaceUsesOfWith(&Loop0LatchBB,
                                                           NewLoop03PHBB);
             AR.DT.addNewBlock(NewLoop03PHBB, &Loop02PHBB);
             AR.DT.addNewBlock(NewLoop03BB, NewLoop03PHBB);
             AR.DT.changeImmediateDominator(AR.DT[&Loop0LatchBB],
                                            AR.DT[NewLoop03BB]);
             AR.DT.verifyDomTree();
             ParentL->addBasicBlockToLoop(NewLoop03PHBB, AR.LI);
-            OldL->addBasicBlockToLoop(NewLoop03BB, AR.LI);
-            OldL->verifyLoop();
+            NewSibling->addBasicBlockToLoop(NewLoop03BB, AR.LI);
+            NewSibling->verifyLoop();
             ParentL->verifyLoop();
-            Updater.addSiblingLoops({OldL});
+            Updater.addSiblingLoops({NewSibling});
             return PreservedAnalyses::all();
           }));
 
   // To respect our inner-to-outer traversal order, we must visit the
   // newly-inserted sibling of the loop we just deleted before we visit the
   // outer loop. When we do so, this must compute a fresh analysis result, even
   // though our new loop has the same pointer value as the loop we deleted.
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.3"), _, _, _))
TEST_F(LoopPassManagerTest, LoopDeletion) {
   AddLoopPipelineAndVerificationPasses();
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _))
       .WillOnce(Invoke(getLoopAnalysisResult));
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _))
       .WillOnce(
           Invoke([&](Loop &L, LoopAnalysisManager &AM,
                      LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
             AR.SE.forgetLoop(&L);
-            delete RemoveLoop(L, Loop00PHBB, AR, Updater);
+            EraseLoop(L, Loop00PHBB, AR, Updater);
             return PreservedAnalyses::all();
           }));
 
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.3"), _, _, _))
       .WillOnce(Invoke(getLoopAnalysisResult));
   EXPECT_CALL(MLPHandle, run(HasName("loop.0.3"), _, _, _))
       .WillOnce(
           Invoke([&](Loop &L, LoopAnalysisManager &AM,
                      LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
             AR.SE.forgetLoop(&L);
-            delete RemoveLoop(L, *NewLoop03PHBB, AR, Updater);
+            EraseLoop(L, *NewLoop03PHBB, AR, Updater);
             return PreservedAnalyses::all();
           }));
 
   EXPECT_CALL(MLPHandle, run(HasName("loop.0"), _, _, _))
       .WillOnce(Invoke(getLoopAnalysisResult));
   EXPECT_CALL(MLPHandle, run(HasName("loop.0"), _, _, _))
       .WillOnce(
           Invoke([&](Loop &L, LoopAnalysisManager &AM,
                      LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
             AR.SE.forgetLoop(&L);
-            delete RemoveLoop(L, EntryBB, AR, Updater);
+            EraseLoop(L, EntryBB, AR, Updater);
             return PreservedAnalyses::all();
           }));
 
   // Add the function pass pipeline now that it is fully built up and run it
   // over the module's one function.
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
   MPM.run(*M, MAM);
 }
 }