Use a BumpPtrAllocator for Loop objects

Summary:
And now that we no longer have to explicitly free() the Loop instances, we can
(with more ease) use the destructor of LoopBase to do what LoopBase::clear() was
doing.

Reviewers: chandlerc

Subscribers: mehdi_amini, mcrosier, llvm-commits

Differential Revision: https://reviews.llvm.org/D38201

llvm-svn: 314375

llvm/include/llvm/Analysis/LoopInfo.h

@@ -46,7 +46,9 @@
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
+#include "llvm/Support/Allocator.h"
 #include <algorithm>
+#include <utility>
 
 namespace llvm {
 
@@ -74,25 +76,16 @@ template <class BlockT, class LoopT> class LoopBase {
 
   SmallPtrSet<const BlockT *, 8> DenseBlockSet;
 
+#if !defined(NDEBUG) || !LLVM_ENABLE_ABI_BREAKING_CHECKS
   /// Indicator that this loop is no longer a valid loop.
   bool IsInvalid = false;
+#endif
 
   LoopBase(const LoopBase<BlockT, LoopT> &) = delete;
   const LoopBase<BlockT, LoopT> &
   operator=(const LoopBase<BlockT, LoopT> &) = delete;
 
-  void clear() {
-    IsInvalid = true;
-    SubLoops.clear();
-    Blocks.clear();
-    DenseBlockSet.clear();
-    ParentLoop = nullptr;
-  }
-
 public:
-  /// This creates an empty loop.
-  LoopBase() : ParentLoop(nullptr) {}
-
   /// 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.
@@ -172,8 +165,15 @@ template <class BlockT, class LoopT> class LoopBase {
     return Blocks.size();
   }
 
-  /// Return true if this loop is no longer valid.
+#ifndef NDEBUG
+  /// Return true if this loop is no longer valid.  The only valid use of this
+  /// helper is "assert(L.isInvalid())" or equivalent, since IsInvalid is set to
+  /// false by the destructor.  In other words, if this accessor returns false,
+  /// the caller has already triggered UB by calling this accessor; and so it
+  /// can only be called in a context where a return value of false indicates a
+  /// programmer error.
   bool isInvalid() const { return IsInvalid; }
+#endif
 
   /// True if terminator in the block can branch to another block that is
   /// outside of the current loop.
@@ -370,6 +370,10 @@ template <class BlockT, class LoopT> class LoopBase {
 
 protected:
   friend class LoopInfoBase<BlockT, LoopT>;
+
+  /// This creates an empty loop.
+  LoopBase() : ParentLoop(nullptr) {}
+
   explicit LoopBase(BlockT *BB) : ParentLoop(nullptr) {
     Blocks.push_back(BB);
     DenseBlockSet.insert(BB);
@@ -386,7 +390,13 @@ template <class BlockT, class LoopT> class LoopBase {
   // non-public.
   ~LoopBase() {
     for (auto *SubLoop : SubLoops)
-      delete SubLoop;
+      SubLoop->~LoopT();
+
+    IsInvalid = true;
+    SubLoops.clear();
+    Blocks.clear();
+    DenseBlockSet.clear();
+    ParentLoop = nullptr;
   }
 };
 
@@ -422,8 +432,6 @@ class Loop : public LoopBase<BasicBlock, Loop> {
     explicit operator bool() const { return Start && End; }
   };
 
-  Loop() {}
-
   /// Return true if the specified value is loop invariant.
   bool isLoopInvariant(const Value *V) const;
 
@@ -534,15 +542,15 @@ class Loop : public LoopBase<BasicBlock, Loop> {
   LocRange getLocRange() const;
 
   StringRef getName() const {
-    if (isInvalid())
-      return "<invalidated loop>";
     if (BasicBlock *Header = getHeader())
       if (Header->hasName())
         return Header->getName();
     return "<unnamed loop>";
   }
 
 private:
+  Loop() = default;
+
   friend class LoopInfoBase<BasicBlock, Loop>;
   friend class LoopBase<BasicBlock, Loop>;
   explicit Loop(BasicBlock *BB) : LoopBase<BasicBlock, Loop>(BB) {}
@@ -558,7 +566,7 @@ template <class BlockT, class LoopT> class LoopInfoBase {
   // BBMap - Mapping of basic blocks to the inner most loop they occur in
   DenseMap<const BlockT *, LoopT *> BBMap;
   std::vector<LoopT *> TopLevelLoops;
-  std::vector<LoopT *> RemovedLoops;
+  BumpPtrAllocator LoopAllocator;
 
   friend class LoopBase<BlockT, LoopT>;
   friend class LoopInfo;
@@ -572,16 +580,19 @@ template <class BlockT, class LoopT> class LoopInfoBase {
 
   LoopInfoBase(LoopInfoBase &&Arg)
       : BBMap(std::move(Arg.BBMap)),
-        TopLevelLoops(std::move(Arg.TopLevelLoops)) {
+        TopLevelLoops(std::move(Arg.TopLevelLoops)),
+        LoopAllocator(std::move(Arg.LoopAllocator)) {
     // We have to clear the arguments top level loops as we've taken ownership.
     Arg.TopLevelLoops.clear();
   }
   LoopInfoBase &operator=(LoopInfoBase &&RHS) {
     BBMap = std::move(RHS.BBMap);
 
     for (auto *L : TopLevelLoops)
-      delete L;
+      L->~LoopT();
+
     TopLevelLoops = std::move(RHS.TopLevelLoops);
+    LoopAllocator = std::move(RHS.LoopAllocator);
     RHS.TopLevelLoops.clear();
     return *this;
   }
@@ -590,11 +601,14 @@ template <class BlockT, class LoopT> class LoopInfoBase {
     BBMap.clear();
 
     for (auto *L : TopLevelLoops)
-      delete L;
+      L->~LoopT();
     TopLevelLoops.clear();
-    for (auto *L : RemovedLoops)
-      delete L;
-    RemovedLoops.clear();
+    LoopAllocator.Reset();
+  }
+
+  template <typename... ArgsTy> LoopT *AllocateLoop(ArgsTy &&... Args) {
+    LoopT *Storage = LoopAllocator.Allocate<LoopT>();
+    return new (Storage) LoopT(std::forward<ArgsTy>(Args)...);
   }
 
   /// iterator/begin/end - The interface to the top-level loops in the current
@@ -717,7 +731,15 @@ template <class BlockT, class LoopT> class LoopInfoBase {
   void verify(const DominatorTreeBase<BlockT, false> &DomTree) const;
 
 protected:
-  static void clearLoop(LoopT &L) { L.clear(); }
+  // Calls the destructor for \p L but keeps the memory for \p L around so that
+  // the pointer value does not get re-used.
+  void destroy(LoopT *L) {
+    L->~LoopT();
+
+    // Since LoopAllocator is a BumpPtrAllocator, this Deallocate only poisons
+    // \c L, but the pointer remains valid for non-dereferencing uses.
+    LoopAllocator.Deallocate(L, sizeof(LoopT));
+  }
 };
 
 // Implementation in LoopInfoImpl.h

llvm/include/llvm/Analysis/LoopInfoImpl.h

@@ -498,7 +498,7 @@ void LoopInfoBase<BlockT, LoopT>::analyze(const DomTreeBase<BlockT> &DomTree) {
     }
     // Perform a backward CFG traversal to discover and map blocks in this loop.
     if (!Backedges.empty()) {
-      LoopT *L = new LoopT(Header);
+      LoopT *L = AllocateLoop(Header);
       discoverAndMapSubloop(L, ArrayRef<BlockT *>(Backedges), this, DomTree);
     }
   }

llvm/include/llvm/Analysis/LoopPass.h

@@ -129,6 +129,9 @@ class LPPassManager : public FunctionPass, public PMDataManager {
   // Add a new loop into the loop queue.
   void addLoop(Loop &L);
 
+  // Mark \p L as deleted.
+  void markLoopAsDeleted(Loop &L);
+
   //===--------------------------------------------------------------------===//
   /// SimpleAnalysis - Provides simple interface to update analysis info
   /// maintained by various passes. Note, if required this interface can
@@ -152,6 +155,7 @@ class LPPassManager : public FunctionPass, public PMDataManager {
   std::deque<Loop *> LQ;
   LoopInfo *LI;
   Loop *CurrentLoop;
+  bool CurrentLoopDeleted;
 };
 
 // This pass is required by the LCSSA transformation. It is used inside

llvm/include/llvm/CodeGen/MachineLoopInfo.h

@@ -44,8 +44,6 @@ extern template class LoopBase<MachineBasicBlock, MachineLoop>;
 
 class MachineLoop : public LoopBase<MachineBasicBlock, MachineLoop> {
 public:
-  MachineLoop();
-
   /// Return the "top" block in the loop, which is the first block in the linear
   /// layout, ignoring any parts of the loop not contiguous with the part that
   /// contains the header.
@@ -76,6 +74,8 @@ class MachineLoop : public LoopBase<MachineBasicBlock, MachineLoop> {
 
   explicit MachineLoop(MachineBasicBlock *MBB)
     : LoopBase<MachineBasicBlock, MachineLoop>(MBB) {}
+
+  MachineLoop() = default;
 };
 
 // Implementation in LoopInfoImpl.h

llvm/include/llvm/IR/PassManager.h

@@ -654,9 +654,9 @@ template <typename IRUnitT, typename... ExtraArgTs> class AnalysisManager {
   /// This doesn't invalidate, but instead simply deletes, the relevant results.
   /// It is useful when the IR is being removed and we want to clear out all the
   /// memory pinned for it.
-  void clear(IRUnitT &IR) {
+  void clear(IRUnitT &IR, llvm::StringRef Name) {
     if (DebugLogging)
-      dbgs() << "Clearing all analysis results for: " << IR.getName() << "\n";
+      dbgs() << "Clearing all analysis results for: " << Name << "\n";
 
     auto ResultsListI = AnalysisResultLists.find(&IR);
     if (ResultsListI == AnalysisResultLists.end())

llvm/include/llvm/Support/Allocator.h

@@ -269,6 +269,9 @@ class BumpPtrAllocatorImpl
   // Pull in base class overloads.
   using AllocatorBase<BumpPtrAllocatorImpl>::Allocate;
 
+  // Bump pointer allocators are expected to never free their storage; and
+  // clients expect pointers to remain valid for non-dereferencing uses even
+  // after deallocation.
   void Deallocate(const void *Ptr, size_t Size) {
     __asan_poison_memory_region(Ptr, Size);
   }

llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

@@ -164,8 +164,8 @@ class LPMUpdater {
   /// If this is called for the current loop, in addition to clearing any
   /// state, this routine will mark that the current loop should be skipped by
   /// the rest of the pass management infrastructure.
-  void markLoopAsDeleted(Loop &L) {
-    LAM.clear(L);
+  void markLoopAsDeleted(Loop &L, llvm::StringRef Name) {
+    LAM.clear(L, Name);
     assert((&L == CurrentL || CurrentL->contains(&L)) &&
            "Cannot delete a loop outside of the "
            "subloop tree currently being processed.");

llvm/lib/Analysis/CGSCCPassManager.cpp

@@ -235,7 +235,7 @@ bool FunctionAnalysisManagerCGSCCProxy::Result::invalidate(
   auto PAC = PA.getChecker<FunctionAnalysisManagerCGSCCProxy>();
   if (!PAC.preserved() && !PAC.preservedSet<AllAnalysesOn<LazyCallGraph::SCC>>()) {
     for (LazyCallGraph::Node &N : C)
-      FAM->clear(N.getFunction());
+      FAM->clear(N.getFunction(), N.getFunction().getName());
 
     return true;
   }

llvm/lib/Analysis/LoopAnalysisManager.cpp

@@ -57,7 +57,7 @@ bool LoopAnalysisManagerFunctionProxy::Result::invalidate(
     // those results. Note that the order doesn't matter here as this will just
     // directly destroy the results without calling methods on them.
     for (Loop *L : PreOrderLoops)
-      InnerAM->clear(*L);
+      InnerAM->clear(*L, L->getName());
 
     // We also need to null out the inner AM so that when the object gets
     // destroyed as invalid we don't try to clear the inner AM again. At that

llvm/lib/Analysis/LoopInfo.cpp

@@ -620,10 +620,8 @@ bool LoopInfo::invalidate(Function &F, const PreservedAnalyses &PA,
 
 void LoopInfo::erase(Loop *Unloop) {
   assert(!Unloop->isInvalid() && "Loop has already been erased!");
-  RemovedLoops.push_back(Unloop);
 
-  auto InvalidateOnExit =
-      make_scope_exit([&]() { BaseT::clearLoop(*Unloop); });
+  auto InvalidateOnExit = make_scope_exit([&]() { destroy(Unloop); });
 
   // First handle the special case of no parent loop to simplify the algorithm.
   if (!Unloop->getParentLoop()) {

llvm/lib/Analysis/LoopPass.cpp

@@ -140,6 +140,13 @@ void LPPassManager::getAnalysisUsage(AnalysisUsage &Info) const {
   Info.setPreservesAll();
 }
 
+void LPPassManager::markLoopAsDeleted(Loop &L) {
+  assert((&L == CurrentLoop || CurrentLoop->contains(&L)) &&
+         "Must not delete loop outside the current loop tree!");
+  if (&L == CurrentLoop)
+    CurrentLoopDeleted = true;
+}
+
 /// run - Execute all of the passes scheduled for execution.  Keep track of
 /// whether any of the passes modifies the function, and if so, return true.
 bool LPPassManager::runOnFunction(Function &F) {
@@ -176,7 +183,7 @@ bool LPPassManager::runOnFunction(Function &F) {
 
   // Walk Loops
   while (!LQ.empty()) {
-    bool LoopWasDeleted = false;
+    CurrentLoopDeleted = false;
     CurrentLoop = LQ.back();
 
     // Run all passes on the current Loop.
@@ -195,13 +202,14 @@ bool LPPassManager::runOnFunction(Function &F) {
 
         Changed |= P->runOnLoop(CurrentLoop, *this);
       }
-      LoopWasDeleted = CurrentLoop->isInvalid();
 
       if (Changed)
-        dumpPassInfo(P, MODIFICATION_MSG, ON_LOOP_MSG, CurrentLoop->getName());
+        dumpPassInfo(P, MODIFICATION_MSG, ON_LOOP_MSG,
+                     CurrentLoopDeleted ? "<deleted loop>"
+                                        : CurrentLoop->getName());
       dumpPreservedSet(P);
 
-      if (LoopWasDeleted) {
+      if (CurrentLoopDeleted) {
         // Notify passes that the loop is being deleted.
         deleteSimpleAnalysisLoop(CurrentLoop);
       } else {
@@ -229,19 +237,20 @@ bool LPPassManager::runOnFunction(Function &F) {
 
       removeNotPreservedAnalysis(P);
       recordAvailableAnalysis(P);
-      removeDeadPasses(P, LoopWasDeleted ? "<deleted>"
-                                         : CurrentLoop->getHeader()->getName(),
+      removeDeadPasses(P,
+                       CurrentLoopDeleted ? "<deleted>"
+                                          : CurrentLoop->getHeader()->getName(),
                        ON_LOOP_MSG);
 
-      if (LoopWasDeleted)
+      if (CurrentLoopDeleted)
         // Do not run other passes on this loop.
         break;
     }
 
     // If the loop was deleted, release all the loop passes. This frees up
     // some memory, and avoids trouble with the pass manager trying to call
     // verifyAnalysis on them.
-    if (LoopWasDeleted) {
+    if (CurrentLoopDeleted) {
       for (unsigned Index = 0; Index < getNumContainedPasses(); ++Index) {
         Pass *P = getContainedPass(Index);
         freePass(P, "<deleted>", ON_LOOP_MSG);
@@ -359,4 +368,3 @@ bool LoopPass::skipLoop(const Loop *L) const {
 char LCSSAVerificationPass::ID = 0;
 INITIALIZE_PASS(LCSSAVerificationPass, "lcssa-verification", "LCSSA Verifier",
                 false, false)
-

llvm/lib/Transforms/IPO/Inliner.cpp

@@ -1044,8 +1044,8 @@ PreservedAnalyses InlinerPass::run(LazyCallGraph::SCC &InitialC,
     FunctionAnalysisManager &FAM =
         AM.getResult<FunctionAnalysisManagerCGSCCProxy>(DeadC, CG)
             .getManager();
-    FAM.clear(*DeadF);
-    AM.clear(DeadC);
+    FAM.clear(*DeadF, DeadF->getName());
+    AM.clear(DeadC, DeadC.getName());
     auto &DeadRC = DeadC.getOuterRefSCC();
     CG.removeDeadFunction(*DeadF);
 

llvm/lib/Transforms/IPO/LoopExtractor.cpp

@@ -80,7 +80,7 @@ INITIALIZE_PASS(SingleLoopExtractor, "loop-extract-single",
 //
 Pass *llvm::createLoopExtractorPass() { return new LoopExtractor(); }
 
-bool LoopExtractor::runOnLoop(Loop *L, LPPassManager &) {
+bool LoopExtractor::runOnLoop(Loop *L, LPPassManager &LPM) {
   if (skipLoop(L))
     return false;
 
@@ -143,6 +143,7 @@ bool LoopExtractor::runOnLoop(Loop *L, LPPassManager &) {
       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.
+      LPM.markLoopAsDeleted(*L);
       LI.erase(L);
     }
     ++NumExtracted;

llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp

@@ -1386,7 +1386,7 @@ void LoopConstrainer::addToParentLoopIfNeeded(ArrayRef<BasicBlock *> BBs) {
 
 Loop *LoopConstrainer::createClonedLoopStructure(Loop *Original, Loop *Parent,
                                                  ValueToValueMapTy &VM) {
-  Loop &New = *new Loop();
+  Loop &New = *LI.AllocateLoop();
   if (Parent)
     Parent->addChildLoop(&New);
   else