From fdb41805142df0ef14ddbb45065a55aa43e0c6ce Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Wed, 7 Jan 2015 01:58:35 +0000 Subject: [PATCH] [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 --- llvm/include/llvm/Analysis/CGSCCPassManager.h | 12 +++-- llvm/include/llvm/IR/PassManager.h | 24 ++++++---- llvm/lib/Analysis/CGSCCPassManager.cpp | 34 +++++++++++--- llvm/lib/IR/PassManager.cpp | 68 ++++++++++++++++++++++----- llvm/test/Other/new-pass-manager.ll | 63 ++++++++++++++++++++----- 5 files changed, 159 insertions(+), 42 deletions(-) diff --git a/llvm/include/llvm/Analysis/CGSCCPassManager.h b/llvm/include/llvm/Analysis/CGSCCPassManager.h index 70b3ea9..c497a81 100644 --- a/llvm/include/llvm/Analysis/CGSCCPassManager.h +++ b/llvm/include/llvm/Analysis/CGSCCPassManager.h @@ -120,7 +120,7 @@ private: void invalidateImpl(void *PassID, LazyCallGraph::SCC &C); /// \brief Invalidate the results for a function.. - void invalidateImpl(LazyCallGraph::SCC &C, const PreservedAnalyses &PA); + PreservedAnalyses invalidateImpl(LazyCallGraph::SCC &C, PreservedAnalyses PA); /// \brief List of function analysis pass IDs and associated concept pointers. /// @@ -343,11 +343,13 @@ public: // We know that the CGSCC pass couldn't have invalidated any other // SCC's analyses (that's the contract of a CGSCC pass), so - // directly handle the CGSCC analysis manager's invalidation here. + // directly handle the CGSCC analysis manager's invalidation here. We + // also update the preserved set of analyses to reflect that invalidated + // analyses are now safe to preserve. // FIXME: This isn't quite correct. We need to handle the case where the // pass updated the CG, particularly some child of the current SCC, and // invalidate its analyses. - CGAM.invalidate(C, PassPA); + PassPA = CGAM.invalidate(C, std::move(PassPA)); // Then intersect the preserved set so that invalidation of module // analyses will eventually occur when the module pass completes. @@ -562,8 +564,10 @@ public: // We know that the function pass couldn't have invalidated any other // function's analyses (that's the contract of a function pass), so // directly handle the function analysis manager's invalidation here. + // Also, update the preserved analyses to reflect that once invalidated + // these can again be preserved. if (FAM) - FAM->invalidate(N->getFunction(), PassPA); + PassPA = FAM->invalidate(N->getFunction(), std::move(PassPA)); // Then intersect the preserved set so that invalidation of module // analyses will eventually occur when the module pass completes. diff --git a/llvm/include/llvm/IR/PassManager.h b/llvm/include/llvm/IR/PassManager.h index 9656a00..8ecc844 100644 --- a/llvm/include/llvm/IR/PassManager.h +++ b/llvm/include/llvm/IR/PassManager.h @@ -92,9 +92,12 @@ public: } /// \brief Mark a particular pass as preserved, adding it to the set. - template void preserve() { + template void preserve() { preserve(PassT::ID()); } + + /// \brief Mark an abstract PassID as preserved, adding it to the set. + void preserve(void *PassID) { if (!areAllPreserved()) - PreservedPassIDs.insert(PassT::ID()); + PreservedPassIDs.insert(PassID); } /// \brief Intersect this set with another in place. @@ -383,8 +386,11 @@ public: /// /// Walk through all of the analyses pertaining to this unit of IR and /// invalidate them unless they are preserved by the PreservedAnalyses set. - void invalidate(IRUnitT IR, const PreservedAnalyses &PA) { - derived_this()->invalidateImpl(IR, PA); + /// We accept the PreservedAnalyses set by value and update it with each + /// analyis pass which has been successfully invalidated and thus can be + /// preserved going forward. The updated set is returned. + PreservedAnalyses invalidate(IRUnitT IR, PreservedAnalyses PA) { + return derived_this()->invalidateImpl(IR, std::move(PA)); } protected: @@ -451,7 +457,7 @@ private: void invalidateImpl(void *PassID, Module &M); /// \brief Invalidate results across a module. - void invalidateImpl(Module &M, const PreservedAnalyses &PA); + PreservedAnalyses invalidateImpl(Module &M, PreservedAnalyses PA); /// \brief Map type from module analysis pass ID to pass result concept /// pointer. @@ -515,7 +521,7 @@ private: void invalidateImpl(void *PassID, Function &F); /// \brief Invalidate the results for a function.. - void invalidateImpl(Function &F, const PreservedAnalyses &PA); + PreservedAnalyses invalidateImpl(Function &F, PreservedAnalyses PA); /// \brief List of function analysis pass IDs and associated concept pointers. /// @@ -738,9 +744,11 @@ public: // We know that the function pass couldn't have invalidated any other // function's analyses (that's the contract of a function pass), so - // directly handle the function analysis manager's invalidation here. + // directly handle the function analysis manager's invalidation here and + // update our preserved set to reflect that these have already been + // handled. if (FAM) - FAM->invalidate(*I, PassPA); + PassPA = FAM->invalidate(*I, std::move(PassPA)); // Then intersect the preserved set so that invalidation of module // analyses will eventually occur when the module pass completes. diff --git a/llvm/lib/Analysis/CGSCCPassManager.cpp b/llvm/lib/Analysis/CGSCCPassManager.cpp index e13f41a..d8f5e6c 100644 --- a/llvm/lib/Analysis/CGSCCPassManager.cpp +++ b/llvm/lib/Analysis/CGSCCPassManager.cpp @@ -29,8 +29,17 @@ PreservedAnalyses CGSCCPassManager::run(LazyCallGraph::SCC &C, dbgs() << "Running CGSCC pass: " << Passes[Idx]->name() << "\n"; PreservedAnalyses PassPA = Passes[Idx]->run(C, 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(C, PassPA); + PassPA = AM->invalidate(C, 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)); } @@ -94,11 +103,11 @@ void CGSCCAnalysisManager::invalidateImpl(void *PassID, LazyCallGraph::SCC &C) { CGSCCAnalysisResults.erase(RI); } -void CGSCCAnalysisManager::invalidateImpl(LazyCallGraph::SCC &C, - const PreservedAnalyses &PA) { +PreservedAnalyses CGSCCAnalysisManager::invalidateImpl(LazyCallGraph::SCC &C, + 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 SCC: " << C.getName() @@ -110,10 +119,15 @@ void CGSCCAnalysisManager::invalidateImpl(LazyCallGraph::SCC &C, CGSCCAnalysisResultListT &ResultsList = CGSCCAnalysisResultLists[&C]; for (CGSCCAnalysisResultListT::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(C, PA)) { if (DebugPM) - dbgs() << "Invalidating CGSCC analysis: " << lookupPass(I->first).name() + dbgs() << "Invalidating CGSCC analysis: " << lookupPass(PassID).name() << "\n"; InvalidatedPassIDs.push_back(I->first); @@ -121,10 +135,18 @@ void CGSCCAnalysisManager::invalidateImpl(LazyCallGraph::SCC &C, } 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()) CGSCCAnalysisResults.erase( std::make_pair(InvalidatedPassIDs.pop_back_val(), &C)); CGSCCAnalysisResultLists.erase(&C); + + return std::move(PA); } char CGSCCAnalysisManagerModuleProxy::PassID; diff --git a/llvm/lib/IR/PassManager.cpp b/llvm/lib/IR/PassManager.cpp index 6905a22..99cee50 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; diff --git a/llvm/test/Other/new-pass-manager.ll b/llvm/test/Other/new-pass-manager.ll index 222a767..d963ca1 100644 --- a/llvm/test/Other/new-pass-manager.ll +++ b/llvm/test/Other/new-pass-manager.ll @@ -195,14 +195,14 @@ ; CHECK-DO-INVALIDATE-FUNCTION-ANALYSIS-RESULTS: Running function analysis: NoOpFunctionAnalysis ; RUN: opt -disable-output -disable-verify -debug-pass-manager -debug-cgscc-pass-manager \ -; RUN: -passes='require,cgscc(require,function(require,invalidate,require),require),require' %s 2>&1 \ +; RUN: -passes='require,module(require,function(require,invalidate,require),require),require' %s 2>&1 \ ; RUN: | FileCheck %s --check-prefix=CHECK-INVALIDATE-ALL ; CHECK-INVALIDATE-ALL: Starting module pass manager run. ; CHECK-INVALIDATE-ALL: Running module pass: No-op Analysis Requirement Pass ; CHECK-INVALIDATE-ALL: Running module analysis: NoOpModuleAnalysis -; CHECK-INVALIDATE-ALL: Starting CGSCC pass manager run. -; CHECK-INVALIDATE-ALL: Running CGSCC pass: No-op Analysis Requirement Pass -; CHECK-INVALIDATE-ALL: Running CGSCC analysis: NoOpCGSCCAnalysis +; CHECK-INVALIDATE-ALL: Starting module pass manager run. +; CHECK-INVALIDATE-ALL: Running module pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-NOT: Running module analysis: NoOpModuleAnalysis ; CHECK-INVALIDATE-ALL: Starting function pass manager run. ; CHECK-INVALIDATE-ALL: Running function pass: No-op Analysis Requirement Pass ; CHECK-INVALIDATE-ALL: Running function analysis: NoOpFunctionAnalysis @@ -213,19 +213,58 @@ ; CHECK-INVALIDATE-ALL: Running function analysis: NoOpFunctionAnalysis ; CHECK-INVALIDATE-ALL: Finished function pass manager run. ; CHECK-INVALIDATE-ALL: Invalidating all non-preserved analyses for function -; CHECK-INVALIDATE-ALL: Invalidating function analysis: NoOpFunctionAnalysis -; CHECK-INVALIDATE-ALL: Invalidating all non-preserved analyses for SCC -; CHECK-INVALIDATE-ALL: Invalidating CGSCC analysis: NoOpCGSCCAnalysis -; CHECK-INVALIDATE-ALL: Running CGSCC pass: No-op Analysis Requirement Pass -; CHECK-INVALIDATE-ALL: Running CGSCC analysis: NoOpCGSCCAnalysis -; CHECK-INVALIDATE-ALL: Finished CGSCC pass manager run. -; CHECK-INVALIDATE-ALL: Invalidating all non-preserved analyses for SCC -; CHECK-INVALIDATE-ALL: Invalidating CGSCC analysis: NoOpCGSCCAnalysis +; CHECK-INVALIDATE-ALL-NOT: Running function analysis: NoOpFunctionAnalysis ; CHECK-INVALIDATE-ALL: Invalidating all non-preserved analyses for module ; CHECK-INVALIDATE-ALL: Invalidating module analysis: NoOpModuleAnalysis ; CHECK-INVALIDATE-ALL: Running module pass: No-op Analysis Requirement Pass ; CHECK-INVALIDATE-ALL: Running module analysis: NoOpModuleAnalysis ; CHECK-INVALIDATE-ALL: Finished module pass manager run. +; CHECK-INVALIDATE-ALL: Invalidating all non-preserved analyses for module +; CHECK-INVALIDATE-ALL-NOT: Invalidating module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL: Running module pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-NOT: Running module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL: Finished module pass manager run. + +; RUN: opt -disable-output -disable-verify -debug-pass-manager -debug-cgscc-pass-manager \ +; RUN: -passes='require,module(require,cgscc(require,function(require,invalidate,require),require),require),require' %s 2>&1 \ +; RUN: | FileCheck %s --check-prefix=CHECK-INVALIDATE-ALL-CG +; CHECK-INVALIDATE-ALL-CG: Starting module pass manager run. +; CHECK-INVALIDATE-ALL-CG: Running module pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG: Running module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL-CG: Starting module pass manager run. +; CHECK-INVALIDATE-ALL-CG: Running module pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG-NOT: Running module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL-CG: Starting CGSCC pass manager run. +; CHECK-INVALIDATE-ALL-CG: Running CGSCC pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG: Running CGSCC analysis: NoOpCGSCCAnalysis +; CHECK-INVALIDATE-ALL-CG: Starting function pass manager run. +; CHECK-INVALIDATE-ALL-CG: Running function pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG: Running function analysis: NoOpFunctionAnalysis +; CHECK-INVALIDATE-ALL-CG: Running function pass: InvalidateAllAnalysesPass +; CHECK-INVALIDATE-ALL-CG: Invalidating all non-preserved analyses for function +; CHECK-INVALIDATE-ALL-CG: Invalidating function analysis: NoOpFunctionAnalysis +; CHECK-INVALIDATE-ALL-CG: Running function pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG: Running function analysis: NoOpFunctionAnalysis +; CHECK-INVALIDATE-ALL-CG: Finished function pass manager run. +; CHECK-INVALIDATE-ALL-CG: Invalidating all non-preserved analyses for function +; CHECK-INVALIDATE-ALL-CG-NOT: Running function analysis: NoOpFunctionAnalysis +; CHECK-INVALIDATE-ALL-CG: Invalidating all non-preserved analyses for SCC +; CHECK-INVALIDATE-ALL-CG: Invalidating CGSCC analysis: NoOpCGSCCAnalysis +; CHECK-INVALIDATE-ALL-CG: Running CGSCC pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG: Running CGSCC analysis: NoOpCGSCCAnalysis +; CHECK-INVALIDATE-ALL-CG: Finished CGSCC pass manager run. +; CHECK-INVALIDATE-ALL-CG: Invalidating all non-preserved analyses for SCC +; CHECK-INVALIDATE-ALL-CG-NOT: Invalidating CGSCC analysis: NoOpCGSCCAnalysis +; CHECK-INVALIDATE-ALL-CG: Invalidating all non-preserved analyses for module +; CHECK-INVALIDATE-ALL-CG: Invalidating module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL-CG: Running module pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG: Running module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL-CG: Finished module pass manager run. +; CHECK-INVALIDATE-ALL-CG: Invalidating all non-preserved analyses for module +; CHECK-INVALIDATE-ALL-CG-NOT: Invalidating module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL-CG: Running module pass: No-op Analysis Requirement Pass +; CHECK-INVALIDATE-ALL-CG-NOT: Running module analysis: NoOpModuleAnalysis +; CHECK-INVALIDATE-ALL-CG: Finished module pass manager run. define void @foo() { ret void -- 2.7.4