]> granicus.if.org Git - llvm/commitdiff
[PH] Replace uses of AssertingVH from members of analysis results with
authorChandler Carruth <chandlerc@gmail.com>
Tue, 24 Jan 2017 12:55:57 +0000 (12:55 +0000)
committerChandler Carruth <chandlerc@gmail.com>
Tue, 24 Jan 2017 12:55:57 +0000 (12:55 +0000)
a lazy-asserting PoisoningVH.

AssertVH is fundamentally incompatible with cache-invalidation of
analysis results. The invaliadtion happens after the AssertingVH has
already fired. Instead, use a PoisoningVH that will assert if the
dangling handle is ever used rather than merely be assigned or
destroyed.

This patch also removes all of the (numerous) doomed attempts to work
around this fundamental incompatibility. It is a pretty significant
simplification IMO.

The most interesting change is in the Inliner where we still do some
clearing because we don't want to rely on the coarse grained
invalidation strategy of the containing pass manager. However, I prefer
the approach that contains this logic to the cleanup phase of the
Inliner, and I think we could enhance the CGSCC analysis management
layer to make this even better in the future if desired.

The rest is straight cleanup.

I've also added a test for one of the harder cases to work around: when
a *module analysis* contains many AssertingVHes pointing at functions.

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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@292928 91177308-0d34-0410-b5e6-96231b3b80d8

13 files changed:
include/llvm/Analysis/CallGraph.h
include/llvm/Analysis/ScalarEvolution.h
lib/Analysis/LazyValueInfo.cpp
lib/Transforms/IPO/FunctionAttrs.cpp
lib/Transforms/IPO/GlobalDCE.cpp
lib/Transforms/IPO/Inliner.cpp
lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
lib/Transforms/Scalar/NaryReassociate.cpp
lib/Transforms/Utils/LoopSimplify.cpp
test/Other/new-pm-defaults.ll
test/Other/new-pm-lto-defaults.ll
test/Transforms/GlobalDCE/crash-assertingvh.ll
test/Transforms/Inline/clear-analyses.ll

index 4ecbaa75ac756785c759cb6c277b75110be5b748..ea85436ee580e49a0559b8766921ea40fdcfad10 100644 (file)
@@ -272,7 +272,7 @@ public:
 private:
   friend class CallGraph;
 
-  AssertingVH<Function> F;
+  Function *F;
 
   std::vector<CallRecord> CalledFunctions;
 
index 1a93f9aa5fd2718019a251981c4f4f75e776d61b..bc2b3a311358477536be3f4e7532a50eabf617a3 100644 (file)
@@ -600,14 +600,14 @@ private:
   /// Information about the number of times a particular loop exit may be
   /// reached before exiting the loop.
   struct ExitNotTakenInfo {
-    AssertingVH<BasicBlock> ExitingBlock;
+    PoisoningVH<BasicBlock> ExitingBlock;
     const SCEV *ExactNotTaken;
     std::unique_ptr<SCEVUnionPredicate> Predicate;
     bool hasAlwaysTruePredicate() const {
       return !Predicate || Predicate->isAlwaysTrue();
     }
 
-    explicit ExitNotTakenInfo(AssertingVH<BasicBlock> ExitingBlock,
+    explicit ExitNotTakenInfo(PoisoningVH<BasicBlock> ExitingBlock,
                               const SCEV *ExactNotTaken,
                               std::unique_ptr<SCEVUnionPredicate> Predicate)
         : ExitingBlock(ExitingBlock), ExactNotTaken(ExactNotTaken),
index dc22b8173a85ddda1c99b51befba5a1809f3c4a8..b2a8e6f3ce5ad0beaf18347f839d5a1fdc3d34ae 100644 (file)
@@ -366,7 +366,7 @@ namespace {
     struct ValueCacheEntryTy {
       ValueCacheEntryTy(Value *V, LazyValueInfoCache *P) : Handle(V, P) {}
       LVIValueHandle Handle;
-      SmallDenseMap<AssertingVH<BasicBlock>, LVILatticeVal, 4> BlockVals;
+      SmallDenseMap<PoisoningVH<BasicBlock>, LVILatticeVal, 4> BlockVals;
     };
 
     /// This is all of the cached information for all values,
@@ -375,13 +375,13 @@ namespace {
 
     /// This tracks, on a per-block basis, the set of values that are
     /// over-defined at the end of that block.
-    typedef DenseMap<AssertingVH<BasicBlock>, SmallPtrSet<Value *, 4>>
+    typedef DenseMap<PoisoningVH<BasicBlock>, SmallPtrSet<Value *, 4>>
         OverDefinedCacheTy;
     OverDefinedCacheTy OverDefinedCache;
 
     /// Keep track of all blocks that we have ever seen, so we
     /// don't spend time removing unused blocks from our caches.
-    DenseSet<AssertingVH<BasicBlock> > SeenBlocks;
+    DenseSet<PoisoningVH<BasicBlock> > SeenBlocks;
 
   public:
     void insertResult(Value *Val, BasicBlock *BB, const LVILatticeVal &Result) {
@@ -464,10 +464,10 @@ void LazyValueInfoCache::eraseValue(Value *V) {
     SmallPtrSetImpl<Value *> &ValueSet = I.second;
     ValueSet.erase(V);
     if (ValueSet.empty())
-      ToErase.push_back(I.first);
+      ToErase.push_back(&*I.first);
   }
   for (auto &BB : ToErase)
-    OverDefinedCache.erase(BB);
+    OverDefinedCache.erase(&*BB);
 
   ValueCache.erase(V);
 }
@@ -480,7 +480,7 @@ void LVIValueHandle::deleted() {
 
 void LazyValueInfoCache::eraseBlock(BasicBlock *BB) {
   // Shortcut if we have never seen this block.
-  DenseSet<AssertingVH<BasicBlock> >::iterator I = SeenBlocks.find(BB);
+  DenseSet<PoisoningVH<BasicBlock> >::iterator I = SeenBlocks.find(BB);
   if (I == SeenBlocks.end())
     return;
   SeenBlocks.erase(I);
index 402a66552c243bfb864c101c2b2ed16ef51fb784..740e9fb8b90d31fb113411c967aaaf72905ca011 100644 (file)
@@ -1275,16 +1275,9 @@ PreservedAnalyses
 ReversePostOrderFunctionAttrsPass::run(Module &M, ModuleAnalysisManager &AM) {
   auto &CG = AM.getResult<CallGraphAnalysis>(M);
 
-  bool Changed = deduceFunctionAttributeInRPO(M, CG);
-
-  // CallGraphAnalysis holds AssertingVH and must be invalidated eagerly so
-  // that other passes don't delete stuff from under it.
-  // FIXME: We need to invalidate this to avoid PR28400. Is there a better
-  // solution?
-  AM.invalidate<CallGraphAnalysis>(M);
-
-  if (!Changed)
+  if (!deduceFunctionAttributeInRPO(M, CG))
     return PreservedAnalyses::all();
+
   PreservedAnalyses PA;
   PA.preserve<CallGraphAnalysis>();
   return PA;
index 9304e783d49256dd9843a00eb4551f730f213b59..67e9d20eb9200a921c0b544a59af2534e544e3ca 100644 (file)
@@ -144,29 +144,13 @@ PreservedAnalyses GlobalDCEPass::run(Module &M, ModuleAnalysisManager &MAM) {
       }
     }
 
-  // Because we may have cached analyses for functions we take extra care when
-  // deleting them if there is an active proxy. If there isn't, then we get to
-  // assume that everything in the function AM has been cleared already.
-  // FIXME: Note that all of this will happen automatically when this pass
-  // finishes. Unfortuantely there are analyses which hold asserting VHs to
-  // IR units. We could make those weak VHs that would assert if ever used
-  // without asserting eagerly and then all of this knowledge of the analysis
-  // manager could go away.
-  FunctionAnalysisManager *FAM = nullptr;
-  if (auto *FAMProxy =
-          MAM.getCachedResult<FunctionAnalysisManagerModuleProxy>(M))
-    FAM = &FAMProxy->getManager();
-
   // The second pass drops the bodies of functions which are dead...
   std::vector<Function *> DeadFunctions;
   for (Function &F : M)
     if (!AliveGlobals.count(&F)) {
       DeadFunctions.push_back(&F);         // Keep track of dead globals
-      if (!F.isDeclaration()) {
-        if (FAM)
-          FAM->clear(F);
+      if (!F.isDeclaration())
         F.deleteBody();
-      }
     }
 
   // The third pass drops targets of aliases which are dead...
index e2dc1158549b3195b7d288e8197061a42e4b65b2..767a2076de6f6a9a654b3e730003161340935195 100644 (file)
@@ -887,11 +887,10 @@ PreservedAnalyses InlinerPass::run(LazyCallGraph::SCC &InitialC,
         // made dead by this operation on other functions).
         Callee.removeDeadConstantUsers();
         if (Callee.use_empty()) {
-          // Clear all analyses and the body and queue the function itself for
-          // deletion when we finish inlining and call graph updates.
+          // Clear the body and queue the function itself for deletion when we
+          // finish inlining and call graph updates.
           // Note that after this point, it is an error to do anything other
           // than use the callee's address or delete it.
-          FAM.clear(Callee);
           Callee.dropAllReferences();
           assert(find(DeadFunctions, &Callee) == DeadFunctions.end() &&
                  "Cannot put cause a function to become dead twice!");
@@ -939,8 +938,13 @@ PreservedAnalyses InlinerPass::run(LazyCallGraph::SCC &InitialC,
   // sets.
   for (Function *DeadF : DeadFunctions) {
     // Get the necessary information out of the call graph and nuke the
-    // function there.
+    // function there. Also, cclear out any cached analyses.
     auto &DeadC = *CG.lookupSCC(*CG.lookup(*DeadF));
+    FunctionAnalysisManager &FAM =
+        AM.getResult<FunctionAnalysisManagerCGSCCProxy>(DeadC, CG)
+            .getManager();
+    FAM.clear(*DeadF);
+    AM.clear(DeadC);
     auto &DeadRC = DeadC.getOuterRefSCC();
     CG.removeDeadFunction(*DeadF);
 
index 3cb5b5d8f7ad71544c35a9e63b3eb9124fc5c815..fd931c521c8f1786297e3015ee9c120f2dfbce94 100644 (file)
@@ -438,13 +438,7 @@ AlignmentFromAssumptionsPass::run(Function &F, FunctionAnalysisManager &AM) {
   AssumptionCache &AC = AM.getResult<AssumptionAnalysis>(F);
   ScalarEvolution &SE = AM.getResult<ScalarEvolutionAnalysis>(F);
   DominatorTree &DT = AM.getResult<DominatorTreeAnalysis>(F);
-  bool Changed = runImpl(F, AC, &SE, &DT);
-
-  // FIXME: We need to invalidate this to avoid PR28400. Is there a better
-  // solution?
-  AM.invalidate<ScalarEvolutionAnalysis>(F);
-
-  if (!Changed)
+  if (!runImpl(F, AC, &SE, &DT))
     return PreservedAnalyses::all();
 
   PreservedAnalyses PA;
index a16ef438d0ea6821436150d0d54a61f6018953f4..c5bf2f28d1852fb406f33a1a88c9b65c593ce1e3 100644 (file)
@@ -156,13 +156,7 @@ PreservedAnalyses NaryReassociatePass::run(Function &F,
   auto *TLI = &AM.getResult<TargetLibraryAnalysis>(F);
   auto *TTI = &AM.getResult<TargetIRAnalysis>(F);
 
-  bool Changed = runImpl(F, AC, DT, SE, TLI, TTI);
-
-  // FIXME: We need to invalidate this to avoid PR28400. Is there a better
-  // solution?
-  AM.invalidate<ScalarEvolutionAnalysis>(F);
-
-  if (!Changed)
+  if (!runImpl(F, AC, DT, SE, TLI, TTI))
     return PreservedAnalyses::all();
 
   PreservedAnalyses PA;
index b60e0a150c7123b85cca975518d1c9fdd8ae3c2a..85bdeecb79cc637ee1ee521c8015f237b5f011b6 100644 (file)
@@ -853,12 +853,9 @@ PreservedAnalyses LoopSimplifyPass::run(Function &F,
   for (LoopInfo::iterator I = LI->begin(), E = LI->end(); I != E; ++I)
     Changed |= simplifyLoop(*I, DT, LI, SE, AC, /*PreserveLCSSA*/ false);
 
-  // FIXME: We need to invalidate this to avoid PR28400. Is there a better
-  // solution?
-  AM.invalidate<ScalarEvolutionAnalysis>(F);
-
   if (!Changed)
     return PreservedAnalyses::all();
+
   PreservedAnalyses PA;
   PA.preserve<DominatorTreeAnalysis>();
   PA.preserve<LoopAnalysis>();
index 225b49afbf71249e57774a367ebb8b075636fb61..52ed25ef656fd412778e80b9476b4ddb68531b17 100644 (file)
@@ -91,8 +91,6 @@
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O-NEXT: Running pass: FunctionToLoopPassAdaptor<{{.*}}LoopStandardAnalysisResults{{.*}}>
-; CHECK-O-NEXT: Invalidating analysis: ScalarEvolutionAnalysis
-; CHECK-O-NEXT: Running analysis: ScalarEvolutionAnalysis
 ; CHECK-O-NEXT: Starting Loop pass manager run.
 ; CHECK-O-NEXT: Finished Loop pass manager run.
 ; CHECK-Os-NEXT: Running pass: MergedLoadStoreMotionPass
 ; CHECK-O-NEXT: Running pass: EliminateAvailableExternallyPass
 ; CHECK-O-NEXT: Running pass: ReversePostOrderFunctionAttrsPass
 ; CHECK-O-NEXT: Running analysis: CallGraphAnalysis
-; CHECK-O-NEXT: Invalidating analysis: CallGraphAnalysis
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: Float2IntPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O-NEXT: Running pass: AlignmentFromAssumptionsPass
-; CHECK-O-NEXT: Invalidating analysis: ScalarEvolutionAnalysis
 ; CHECK-O-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: ConstantMergePass
index 8f54a7dfdf70fbcc5d413388af6447615e3a9783..e86bcdddcdaeaf65a15f2f9a2d9fc09dd66f64a9 100644 (file)
@@ -37,7 +37,6 @@
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: ReversePostOrderFunctionAttrsPass
 ; CHECK-O-NEXT: Running analysis: CallGraphAnalysis
-; CHECK-O-NEXT: Invalidating analysis: CallGraphAnalysis
 ; CHECK-O-NEXT: Running pass: GlobalSplitPass
 ; CHECK-O-NEXT: Running pass: WholeProgramDevirtPass
 ; CHECK-O2-NEXT: Running pass: GlobalOptPass
index 184d6ae7d8e9600b5aba85010ee74ab1858d9c3b..2919999d5e2880b00b28de5e71c8b9bc8493da1a 100644 (file)
@@ -3,6 +3,7 @@
 ; to assert when global DCE deletes the body of the function.
 ;
 ; RUN: opt -disable-output < %s -passes='module(function(jump-threading),globaldce)'
+; RUN: opt -disable-output < %s -passes='module(rpo-functionattrs,globaldce)'
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
index 1fde513f282d2139defb67b5fffa1f2bc867dcba..4b1d37ca29a98fa7815c1ac5353e6a9f95db7284 100644 (file)
@@ -1,8 +1,7 @@
-; Test that the inliner clears analyses which may hold references to function
-; bodies when it decides to delete them after inlining the last caller.
-; We check this by using correlated-propagation to populate LVI with basic
-; block references that would dangle if we failed to clear the inlined function
-; body.
+; Test that when a pass like correlated-propagation populates an analysis such
+; as LVI with references back into the IR of a function that the inliner will
+; delete, this doesn't crash or go awry despite the inliner clearing the analyses
+; separately from when it deletes the function.
 ;
 ; RUN: opt -debug-pass-manager -S < %s 2>&1 \
 ; RUN:     -passes='cgscc(inline,function(correlated-propagation))' \