[PM] Fix a pretty nasty bug where the new pass manager would invalidate

passes too many time.

I think this is actually the issue that someone raised with me at the
developer's meeting and in an email, but that we never really got to the
bottom of. Having all the testing utilities made it much easier to dig
down and uncover the core issue.

When a pass manager is running many passes over a single function, we
need it to invalidate the analyses between each run so that they can be
re-computed as needed. We also need to track the intersection of
preserved higher-level analyses across all the passes that we run (for
example, if there is one module analysis which all the function analyses
preserve, we want to track that and propagate it). Unfortunately, this
interacted poorly with any enclosing pass adaptor between two IR units.
It would see the intersection of preserved analyses, and need to
invalidate any other analyses, but some of the un-preserved analyses
might have already been invalidated *and recomputed*! We would fail to
propagate the fact that the analysis had already been invalidated.

The solution to this struck me as really strange at first, but the more
I thought about it, the more natural it seemed. After a nice discussion
with Duncan about it on IRC, it seemed even nicer. The idea is that
invalidating an analysis *causes* it to be preserved! Preserving the
lack of result is trivial. If it is recomputed, great. Until something
*else* invalidates it again, we're good.

The consequence of this is that the invalidate methods on the analysis
manager which operate over many passes now consume their
PreservedAnalyses object, update it to "preserve" every analysis pass to
which it delivers an invalidation (regardless of whether the pass
chooses to be removed, or handles the invalidation itself by updating
itself). Then we return this augmented set from the invalidate routine,
letting the pass manager take the result and use the intersection of
*that* across each pass run to compute the final preserved set. This
accounts for all the places where the early invalidation of an analysis
has already "preserved" it for a future run.

I've beefed up the testing and adjusted the assertions to show that we
no longer repeatedly invalidate or compute the analyses across nested
pass managers.

llvm-svn: 225333

1 files changed, 56 insertions, 12 deletions

diff --git a/llvm/lib/IR/PassManager.cpp b/llvm/lib/IR/PassManager.cpp
index 6905a226fec..99cee507e65 100644
--- a/llvm/lib/IR/PassManager.cpp
+++ b/llvm/lib/IR/PassManager.cpp
@@ -30,8 +30,17 @@ PreservedAnalyses ModulePassManager::run(Module &M, ModuleAnalysisManager *AM) {
       dbgs() << "Running module pass: " << Passes[Idx]->name() << "\n";
 
     PreservedAnalyses PassPA = Passes[Idx]->run(M, AM);
+
+    // If we have an active analysis manager at this level we want to ensure we
+    // update it as each pass runs and potentially invalidates analyses. We
+    // also update the preserved set of analyses based on what analyses we have
+    // already handled the invalidation for here and don't need to invalidate
+    // when finished.
     if (AM)
-      AM->invalidate(M, PassPA);
+      PassPA = AM->invalidate(M, std::move(PassPA));
+
+    // Finally, we intersect the final preserved analyses to compute the
+    // aggregate preserved set for this pass manager.
     PA.intersect(std::move(PassPA));
 
     M.getContext().yield();
@@ -76,11 +85,11 @@ void ModuleAnalysisManager::invalidateImpl(void *PassID, Module &M) {
   ModuleAnalysisResults.erase(PassID);
 }
 
-void ModuleAnalysisManager::invalidateImpl(Module &M,
-                                           const PreservedAnalyses &PA) {
+PreservedAnalyses ModuleAnalysisManager::invalidateImpl(Module &M,
+                                                        PreservedAnalyses PA) {
   // Short circuit for a common case of all analyses being preserved.
   if (PA.areAllPreserved())
-    return;
+    return std::move(PA);
 
   if (DebugPM)
     dbgs() << "Invalidating all non-preserved analyses for module: "
@@ -90,14 +99,27 @@ void ModuleAnalysisManager::invalidateImpl(Module &M,
   // invalidate iteration for DenseMap.
   for (ModuleAnalysisResultMapT::iterator I = ModuleAnalysisResults.begin(),
                                           E = ModuleAnalysisResults.end();
-       I != E; ++I)
+       I != E; ++I) {
+    void *PassID = I->first;
+
+    // Pass the invalidation down to the pass itself to see if it thinks it is
+    // necessary. The analysis pass can return false if no action on the part
+    // of the analysis manager is required for this invalidation event.
     if (I->second->invalidate(M, PA)) {
       if (DebugPM)
         dbgs() << "Invalidating module analysis: "
-               << lookupPass(I->first).name() << "\n";
+               << lookupPass(PassID).name() << "\n";
 
       ModuleAnalysisResults.erase(I);
     }
+
+    // After handling each pass, we mark it as preserved. Once we've
+    // invalidated any stale results, the rest of the system is allowed to
+    // start preserving this analysis again.
+    PA.preserve(PassID);
+  }
+
+  return std::move(PA);
 }
 
 PreservedAnalyses FunctionPassManager::run(Function &F,
@@ -112,8 +134,17 @@ PreservedAnalyses FunctionPassManager::run(Function &F,
       dbgs() << "Running function pass: " << Passes[Idx]->name() << "\n";
 
     PreservedAnalyses PassPA = Passes[Idx]->run(F, AM);
+
+    // If we have an active analysis manager at this level we want to ensure we
+    // update it as each pass runs and potentially invalidates analyses. We
+    // also update the preserved set of analyses based on what analyses we have
+    // already handled the invalidation for here and don't need to invalidate
+    // when finished.
     if (AM)
-      AM->invalidate(F, PassPA);
+      PassPA = AM->invalidate(F, std::move(PassPA));
+
+    // Finally, we intersect the final preserved analyses to compute the
+    // aggregate preserved set for this pass manager.
     PA.intersect(std::move(PassPA));
 
     F.getContext().yield();
@@ -179,11 +210,11 @@ void FunctionAnalysisManager::invalidateImpl(void *PassID, Function &F) {
   FunctionAnalysisResults.erase(RI);
 }
 
-void FunctionAnalysisManager::invalidateImpl(Function &F,
-                                             const PreservedAnalyses &PA) {
+PreservedAnalyses
+FunctionAnalysisManager::invalidateImpl(Function &F, PreservedAnalyses PA) {
   // Short circuit for a common case of all analyses being preserved.
   if (PA.areAllPreserved())
-    return;
+    return std::move(PA);
 
   if (DebugPM)
     dbgs() << "Invalidating all non-preserved analyses for function: "
@@ -195,22 +226,35 @@ void FunctionAnalysisManager::invalidateImpl(Function &F,
   FunctionAnalysisResultListT &ResultsList = FunctionAnalysisResultLists[&F];
   for (FunctionAnalysisResultListT::iterator I = ResultsList.begin(),
                                              E = ResultsList.end();
-       I != E;)
+       I != E;) {
+    void *PassID = I->first;
+
+    // Pass the invalidation down to the pass itself to see if it thinks it is
+    // necessary. The analysis pass can return false if no action on the part
+    // of the analysis manager is required for this invalidation event.
     if (I->second->invalidate(F, PA)) {
       if (DebugPM)
         dbgs() << "Invalidating function analysis: "
-               << lookupPass(I->first).name() << "\n";
+               << lookupPass(PassID).name() << "\n";
 
       InvalidatedPassIDs.push_back(I->first);
       I = ResultsList.erase(I);
     } else {
       ++I;
     }
+
+    // After handling each pass, we mark it as preserved. Once we've
+    // invalidated any stale results, the rest of the system is allowed to
+    // start preserving this analysis again.
+    PA.preserve(PassID);
+  }
   while (!InvalidatedPassIDs.empty())
     FunctionAnalysisResults.erase(
         std::make_pair(InvalidatedPassIDs.pop_back_val(), &F));
   if (ResultsList.empty())
     FunctionAnalysisResultLists.erase(&F);
+
+  return std::move(PA);
 }
 
 char FunctionAnalysisManagerModuleProxy::PassID;