Index: include/llvm/IR/LegacyPassManagers.h =================================================================== --- include/llvm/IR/LegacyPassManagers.h +++ include/llvm/IR/LegacyPassManagers.h @@ -186,6 +186,14 @@ /// Set pass P as the last user of the given analysis passes. void setLastUser(ArrayRef AnalysisPasses, Pass *P); + /// Set pass P as locked for last use changes. This is used to make sure the + /// LastUser indication for a pass won't move past the point where it is not + /// preserved, since this would create inconsistencies. + void setLockedLastUse(const Pass *P); + + /// Determine if the last use of pass P is unlocked. + bool isLastUseUnlocked(const Pass *P); + /// Collect passes whose last user is P void collectLastUses(SmallVectorImpl &LastUses, Pass *P); @@ -235,13 +243,17 @@ /// by this pass manager SmallVector IndirectPassManagers; - // Map to keep track of last user of the analysis pass. - // LastUser->second is the last user of Lastuser->first. + /// Map to keep track of last user of the analysis pass. + /// LastUser->second is the last user of Lastuser->first. DenseMap LastUser; - // Map to keep track of passes that are last used by a pass. - // This inverse map is initialized at PM->run() based on - // LastUser map. + /// Set to keep track of locks on last use. A pass has its last use information + /// locked when the pass is no longer preserved. + SmallPtrSet LockedLastUse; + + /// Map to keep track of passes that are last used by a pass. + /// This inverse map is initialized at PM->run() based on + /// LastUser map. DenseMap > InversedLastUser; /// Immutable passes are managed by top level manager. @@ -420,6 +432,10 @@ bool isPassDebuggingExecutionsOrMore() const; private: + void removeNotPreservedAnalysis(Pass *P, + const AnalysisUsage::VectorType &PreservedSet, + DenseMap &AnalysisSet); + void dumpAnalysisUsage(StringRef Msg, const Pass *P, const AnalysisUsage::VectorType &Set) const; Index: lib/IR/LegacyPassManager.cpp =================================================================== --- lib/IR/LegacyPassManager.cpp +++ lib/IR/LegacyPassManager.cpp @@ -497,6 +497,16 @@ activeStack.push(PMDM); } +/// Set pass P as locked for last use changes. +void PMTopLevelManager::setLockedLastUse(const Pass *P) { + LockedLastUse.insert(P); +} + +/// Determine if the last use of pass P is unlocked. +bool PMTopLevelManager::isLastUseUnlocked(const Pass *P) { + return LockedLastUse.count(P) == 0; +} + /// Set pass P as the last user of the given analysis passes. void PMTopLevelManager::setLastUser(ArrayRef AnalysisPasses, Pass *P) { @@ -505,7 +515,8 @@ PDepth = P->getResolver()->getPMDataManager().getDepth(); for (Pass *AP : AnalysisPasses) { - LastUser[AP] = P; + if (isLastUseUnlocked(AP)) + LastUser[AP] = P; if (P == AP) continue; @@ -543,7 +554,8 @@ if (LUI->second == AP) // DenseMap iterator is not invalidated here because // this is just updating existing entries. - LastUser[LUI->first] = P; + if (isLastUseUnlocked(LUI->first)) + LastUser[LUI->first] = P; } } } @@ -870,28 +882,35 @@ } } -/// Remove Analysis not preserved by Pass P -void PMDataManager::removeNotPreservedAnalysis(Pass *P) { - AnalysisUsage *AnUsage = TPM->findAnalysisUsage(P); - if (AnUsage->getPreservesAll()) - return; - - const AnalysisUsage::VectorType &PreservedSet = AnUsage->getPreservedSet(); - for (DenseMap::iterator I = AvailableAnalysis.begin(), - E = AvailableAnalysis.end(); I != E; ) { +void PMDataManager::removeNotPreservedAnalysis(Pass *P, + const AnalysisUsage::VectorType &PreservedSet, + DenseMap &AnalysisSet) { + for (DenseMap::iterator I = AnalysisSet.begin(), + E = AnalysisSet.end(); I != E; ) { DenseMap::iterator Info = I++; if (Info->second->getAsImmutablePass() == nullptr && std::find(PreservedSet.begin(), PreservedSet.end(), Info->first) == PreservedSet.end()) { // Remove this analysis + Pass *S = Info->second; if (PassDebugging >= Details) { - Pass *S = Info->second; dbgs() << " -- '" << P->getPassName() << "' is not preserving '"; dbgs() << S->getPassName() << "'\n"; } - AvailableAnalysis.erase(Info); + TPM->setLockedLastUse(S); + AnalysisSet.erase(Info); } } +} + +/// Remove Analysis not preserved by Pass P +void PMDataManager::removeNotPreservedAnalysis(Pass *P) { + AnalysisUsage *AnUsage = TPM->findAnalysisUsage(P); + if (AnUsage->getPreservesAll()) + return; + + const AnalysisUsage::VectorType &PreservedSet = AnUsage->getPreservedSet(); + removeNotPreservedAnalysis(P, PreservedSet, AvailableAnalysis); // Check inherited analysis also. If P is not preserving analysis // provided by parent manager then remove it here. @@ -900,22 +919,7 @@ if (!InheritedAnalysis[Index]) continue; - for (DenseMap::iterator - I = InheritedAnalysis[Index]->begin(), - E = InheritedAnalysis[Index]->end(); I != E; ) { - DenseMap::iterator Info = I++; - if (Info->second->getAsImmutablePass() == nullptr && - std::find(PreservedSet.begin(), PreservedSet.end(), Info->first) == - PreservedSet.end()) { - // Remove this analysis - if (PassDebugging >= Details) { - Pass *S = Info->second; - dbgs() << " -- '" << P->getPassName() << "' is not preserving '"; - dbgs() << S->getPassName() << "'\n"; - } - InheritedAnalysis[Index]->erase(Info); - } - } + removeNotPreservedAnalysis(P, PreservedSet, *InheritedAnalysis[Index]); } } Index: test/Feature/legacypassmanager.ll =================================================================== --- /dev/null +++ test/Feature/legacypassmanager.ll @@ -0,0 +1,19 @@ +; RUN: opt < %s -S -globals-aa -lcssa -argpromotion +; +; This is to verify a fix to bug 27050. The opt arguments expands to a series +; that involves -basiccg twice. Before the fix, both those CallGraphs were +; live at the same time during execution, which caused a value handle assert +; during argpromotion since only the latest CG's asserting value handles were +; removed when a function was deleted. + +%rec = type { i8 } + +define void @bar() { + call void @foo(%rec* undef) + ret void +} + +define internal void @foo(%rec* %par0) { + ret void +} + Index: unittests/IR/LegacyPassManagerTest.cpp =================================================================== --- unittests/IR/LegacyPassManagerTest.cpp +++ unittests/IR/LegacyPassManagerTest.cpp @@ -43,6 +43,9 @@ void initializeCGPassPass(PassRegistry&); void initializeLPassPass(PassRegistry&); void initializeBPassPass(PassRegistry&); + void initializeModuleDupePass(PassRegistry&); + void initializeModuleDupeUserPass(PassRegistry&); + void initializeModuleDupePreservationBreakerPass(PassRegistry&); namespace { // ND = no deps @@ -540,6 +543,107 @@ return mod; } + + // This verifies that the locking of LastUser propagation works. + struct ModuleDupe : public ModulePass { + public: + static char run; + static char ID; + static char allocated; + ModuleDupe() : ModulePass(ID) { + initializeModuleDupePass(*PassRegistry::getPassRegistry()); + } + bool runOnModule(Module &M) override { + EXPECT_EQ(allocated, 0); // This is the core expect! + run++; + allocated++; + return true; + } + void releaseMemory() override { + EXPECT_GT(run, 0); + EXPECT_GT(allocated, 0); + allocated--; + } + void getAnalysisUsage(AnalysisUsage &AU) const override { + AU.setPreservesAll(); + } + }; + char ModuleDupe::ID=0; + char ModuleDupe::run=0; + char ModuleDupe::allocated=0; + + struct ModuleDupePreservationBreaker : public ModulePass { + public: + static char run; + static char ID; + ModuleDupePreservationBreaker() : ModulePass(ID) { + initializeModuleDupePreservationBreakerPass(*PassRegistry::getPassRegistry()); + } + bool runOnModule(Module &M) override { + run++; + return true; + } + void getAnalysisUsage(AnalysisUsage &AU) const override { + AU.addRequired(); + } + }; + char ModuleDupePreservationBreaker::ID=0; + char ModuleDupePreservationBreaker::run=0; + + struct ModuleDupeUser: public ModulePass { + public: + static char run; + static char ID; + ModuleDupeUser() : ModulePass(ID) { + initializeModuleDupeUserPass(*PassRegistry::getPassRegistry()); + } + bool runOnModule(Module &M) override { + run++; + return false; + } + void getAnalysisUsage(AnalysisUsage &AU) const override { + AU.addRequired(); + AU.addRequired(); + AU.setPreservesAll(); + } + }; + char ModuleDupeUser::ID=0; + char ModuleDupeUser::run=0; + + TEST(PassManager, CheckOldUnpreservedPassHasBeenRemovedBeforeNewIsRun) { + LLVMContext Context; + Module M("test-checkoldunpreservedpasshasbeenremoved", Context); + struct ModuleDupe *mDupe = new ModuleDupe(); + struct ModuleDupePreservationBreaker *mDPB = new ModuleDupePreservationBreaker(); + struct ModuleDupeUser *mDupeUser = new ModuleDupeUser(); + + mDupe->run = mDupeUser->run = mDPB->run = 0; + + legacy::PassManager Passes; + Passes.add(mDupe); + Passes.add(mDPB); + Passes.add(mDupeUser); + + // The mDPB is not preserving mDupe, so a new Dupe will be scheduled + // before DupeUser. This means we'll have this sequence of passes: + // Dupe + // DupePreservationBreaker + // Dupe + // DupeUser + // + // The DupeUser is requiring DupePreservationBreaker, which tries to trick + // the LegacyPassManager to transitively move the LastUse of the first + // Dupe to DupeUser. This would make it not remove the first Dupe before + // creating the second one, which would risk errors when we have two + // active Dupe instances simultaneously. Dupe has a check for allocation + // that verifies that this doesn't happen. + + Passes.run(M); + EXPECT_EQ(2, mDupe->run); + EXPECT_EQ(1, mDPB->run); + EXPECT_EQ(1, mDupeUser->run); + } + } } @@ -552,3 +656,6 @@ INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass) INITIALIZE_PASS_END(LPass, "lp","lp", false, false) INITIALIZE_PASS(BPass, "bp","bp", false, false) +INITIALIZE_PASS(ModuleDupe, "mdupe", "mdupe", false, false) +INITIALIZE_PASS(ModuleDupeUser, "mdupeuser", "mdupeuser", false, false) +INITIALIZE_PASS(ModuleDupePreservationBreaker, "mdupepb", "mdupepb", false, false)