diff --git a/include/llvm/Analysis/LoopInfo.h b/include/llvm/Analysis/LoopInfo.h index 70ce9a87051..a4e5349daef 100644 --- a/include/llvm/Analysis/LoopInfo.h +++ b/include/llvm/Analysis/LoopInfo.h @@ -85,10 +85,6 @@ class LoopBase { public: /// This creates an empty loop. LoopBase() : ParentLoop(nullptr) {} - ~LoopBase() { - for (size_t i = 0, e = SubLoops.size(); i != e; ++i) - delete SubLoops[i]; - } /// 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 @@ -343,6 +339,20 @@ protected: Blocks.push_back(BB); DenseBlockSet.insert(BB); } + + // Since loop passes like SCEV are allowed to key analysis results off of + // `Loop` pointers, we cannot re-use pointers within a loop pass manager. + // This means loop passes should not be `delete` ing `Loop` objects directly + // (and risk a later `Loop` allocation re-using the address of a previous one) + // but should be using LoopInfo::markAsRemoved, which keeps around the `Loop` + // pointer till the end of the lifetime of the `LoopInfo` object. + // + // To make it easier to follow this rule, we mark the destructor as + // non-public. + ~LoopBase() { + for (auto *SubLoop : SubLoops) + delete SubLoop; + } }; template @@ -500,7 +510,9 @@ public: private: friend class LoopInfoBase; + friend class LoopBase; explicit Loop(BasicBlock *BB) : LoopBase(BB) {} + ~Loop() = default; }; //===----------------------------------------------------------------------===// @@ -702,7 +714,7 @@ public: /// the loop forest and parent loops for each block so that \c L is no longer /// referenced, but does not actually delete \c L immediately. The pointer /// will remain valid until this LoopInfo's memory is released. - void markAsRemoved(Loop *L); + void markAsErased(Loop *L); /// Returns true if replacing From with To everywhere is guaranteed to /// preserve LCSSA form. diff --git a/lib/Analysis/LoopInfo.cpp b/lib/Analysis/LoopInfo.cpp index 697b58622bb..4b829e4e83d 100644 --- a/lib/Analysis/LoopInfo.cpp +++ b/lib/Analysis/LoopInfo.cpp @@ -622,8 +622,8 @@ bool LoopInfo::invalidate(Function &F, const PreservedAnalyses &PA, PAC.preservedSet()); } -void LoopInfo::markAsRemoved(Loop *Unloop) { - assert(!Unloop->isInvalid() && "Loop has already been removed"); +void LoopInfo::markAsErased(Loop *Unloop) { + assert(!Unloop->isInvalid() && "Loop has already been erased!"); Unloop->invalidate(); RemovedLoops.push_back(Unloop); diff --git a/lib/Transforms/IPO/LoopExtractor.cpp b/lib/Transforms/IPO/LoopExtractor.cpp index c74b0a35e29..c1d46f1ba4c 100644 --- a/lib/Transforms/IPO/LoopExtractor.cpp +++ b/lib/Transforms/IPO/LoopExtractor.cpp @@ -143,7 +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. - LI.markAsRemoved(L); + LI.markAsErased(L); } ++NumExtracted; } diff --git a/lib/Transforms/Scalar/LoopDeletion.cpp b/lib/Transforms/Scalar/LoopDeletion.cpp index cf452aa6396..0a3e306a508 100644 --- a/lib/Transforms/Scalar/LoopDeletion.cpp +++ b/lib/Transforms/Scalar/LoopDeletion.cpp @@ -334,7 +334,7 @@ static void deleteDeadLoop(Loop *L, DominatorTree &DT, ScalarEvolution &SE, LI.removeBlock(BB); // The last step is to update LoopInfo now that we've eliminated this loop. - LI.markAsRemoved(L); + LI.markAsErased(L); } PreservedAnalyses LoopDeletionPass::run(Loop &L, LoopAnalysisManager &AM, diff --git a/lib/Transforms/Utils/LoopUnroll.cpp b/lib/Transforms/Utils/LoopUnroll.cpp index 1fdc5e124e5..a3643e35ad4 100644 --- a/lib/Transforms/Utils/LoopUnroll.cpp +++ b/lib/Transforms/Utils/LoopUnroll.cpp @@ -816,7 +816,7 @@ bool llvm::UnrollLoop(Loop *L, unsigned Count, unsigned TripCount, bool Force, Loop *OuterL = L->getParentLoop(); // Update LoopInfo if the loop is completely removed. if (CompletelyUnroll) - LI->markAsRemoved(L); + LI->markAsErased(L); // After complete unrolling most of the blocks should be contained in OuterL. // However, some of them might happen to be out of OuterL (e.g. if they @@ -841,7 +841,7 @@ bool llvm::UnrollLoop(Loop *L, unsigned Count, unsigned TripCount, bool Force, if (NeedToFixLCSSA) { // LCSSA must be performed on the outermost affected loop. The unrolled // loop's last loop latch is guaranteed to be in the outermost loop - // after LoopInfo's been updated by markAsRemoved. + // after LoopInfo's been updated by markAsErased. Loop *LatchLoop = LI->getLoopFor(Latches.back()); Loop *FixLCSSALoop = OuterL; if (!FixLCSSALoop->contains(LatchLoop)) diff --git a/unittests/Analysis/ScalarEvolutionTest.cpp b/unittests/Analysis/ScalarEvolutionTest.cpp index 6f689fad4b5..f1a5a1a6e3b 100644 --- a/unittests/Analysis/ScalarEvolutionTest.cpp +++ b/unittests/Analysis/ScalarEvolutionTest.cpp @@ -110,147 +110,6 @@ TEST_F(ScalarEvolutionsTest, SCEVUnknownRAUW) { EXPECT_EQ(cast(M2->getOperand(1))->getValue(), V0); } -TEST_F(ScalarEvolutionsTest, SCEVMultiplyAddRecs) { - Type *Ty = Type::getInt32Ty(Context); - SmallVector Types; - Types.append(10, Ty); - FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context), Types, false); - Function *F = cast(M.getOrInsertFunction("f", FTy)); - BasicBlock *BB = BasicBlock::Create(Context, "entry", F); - ReturnInst::Create(Context, nullptr, BB); - - ScalarEvolution SE = buildSE(*F); - - // It's possible to produce an empty loop through the default constructor, - // but you can't add any blocks to it without a LoopInfo pass. - Loop L; - const_cast&>(L.getBlocks()).push_back(BB); - - Function::arg_iterator AI = F->arg_begin(); - SmallVector A; - A.push_back(SE.getSCEV(&*AI++)); - A.push_back(SE.getSCEV(&*AI++)); - A.push_back(SE.getSCEV(&*AI++)); - A.push_back(SE.getSCEV(&*AI++)); - A.push_back(SE.getSCEV(&*AI++)); - const SCEV *A_rec = SE.getAddRecExpr(A, &L, SCEV::FlagAnyWrap); - - SmallVector B; - B.push_back(SE.getSCEV(&*AI++)); - B.push_back(SE.getSCEV(&*AI++)); - B.push_back(SE.getSCEV(&*AI++)); - B.push_back(SE.getSCEV(&*AI++)); - B.push_back(SE.getSCEV(&*AI++)); - const SCEV *B_rec = SE.getAddRecExpr(B, &L, SCEV::FlagAnyWrap); - - /* Spot check that we perform this transformation: - {A0,+,A1,+,A2,+,A3,+,A4} * {B0,+,B1,+,B2,+,B3,+,B4} = - {A0*B0,+, - A1*B0 + A0*B1 + A1*B1,+, - A2*B0 + 2A1*B1 + A0*B2 + 2A2*B1 + 2A1*B2 + A2*B2,+, - A3*B0 + 3A2*B1 + 3A1*B2 + A0*B3 + 3A3*B1 + 6A2*B2 + 3A1*B3 + 3A3*B2 + - 3A2*B3 + A3*B3,+, - A4*B0 + 4A3*B1 + 6A2*B2 + 4A1*B3 + A0*B4 + 4A4*B1 + 12A3*B2 + 12A2*B3 + - 4A1*B4 + 6A4*B2 + 12A3*B3 + 6A2*B4 + 4A4*B3 + 4A3*B4 + A4*B4,+, - 5A4*B1 + 10A3*B2 + 10A2*B3 + 5A1*B4 + 20A4*B2 + 30A3*B3 + 20A2*B4 + - 30A4*B3 + 30A3*B4 + 20A4*B4,+, - 15A4*B2 + 20A3*B3 + 15A2*B4 + 60A4*B3 + 60A3*B4 + 90A4*B4,+, - 35A4*B3 + 35A3*B4 + 140A4*B4,+, - 70A4*B4} - */ - - const SCEVAddRecExpr *Product = - dyn_cast(SE.getMulExpr(A_rec, B_rec)); - ASSERT_TRUE(Product); - ASSERT_EQ(Product->getNumOperands(), 9u); - - SmallVector Sum; - Sum.push_back(SE.getMulExpr(A[0], B[0])); - EXPECT_EQ(Product->getOperand(0), SE.getAddExpr(Sum)); - Sum.clear(); - - // SCEV produces different an equal but different expression for these. - // Re-enable when PR11052 is fixed. -#if 0 - Sum.push_back(SE.getMulExpr(A[1], B[0])); - Sum.push_back(SE.getMulExpr(A[0], B[1])); - Sum.push_back(SE.getMulExpr(A[1], B[1])); - EXPECT_EQ(Product->getOperand(1), SE.getAddExpr(Sum)); - Sum.clear(); - - Sum.push_back(SE.getMulExpr(A[2], B[0])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[1], B[1])); - Sum.push_back(SE.getMulExpr(A[0], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[2], B[1])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[1], B[2])); - Sum.push_back(SE.getMulExpr(A[2], B[2])); - EXPECT_EQ(Product->getOperand(2), SE.getAddExpr(Sum)); - Sum.clear(); - - Sum.push_back(SE.getMulExpr(A[3], B[0])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[2], B[1])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[1], B[2])); - Sum.push_back(SE.getMulExpr(A[0], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[3], B[1])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[1], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[3], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[2], B[3])); - Sum.push_back(SE.getMulExpr(A[3], B[3])); - EXPECT_EQ(Product->getOperand(3), SE.getAddExpr(Sum)); - Sum.clear(); - - Sum.push_back(SE.getMulExpr(A[4], B[0])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[3], B[1])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[1], B[3])); - Sum.push_back(SE.getMulExpr(A[0], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[4], B[1])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[3], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[2], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[1], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[4], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[3], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[4], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[3], B[4])); - Sum.push_back(SE.getMulExpr(A[4], B[4])); - EXPECT_EQ(Product->getOperand(4), SE.getAddExpr(Sum)); - Sum.clear(); - - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 5), A[4], B[1])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 10), A[3], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 10), A[2], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 5), A[1], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[4], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[3], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[2], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[4], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[3], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[4], B[4])); - EXPECT_EQ(Product->getOperand(5), SE.getAddExpr(Sum)); - Sum.clear(); - - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 15), A[4], B[2])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[3], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 15), A[2], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 60), A[4], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 60), A[3], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 90), A[4], B[4])); - EXPECT_EQ(Product->getOperand(6), SE.getAddExpr(Sum)); - Sum.clear(); - - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 35), A[4], B[3])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 35), A[3], B[4])); - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 140), A[4], B[4])); - EXPECT_EQ(Product->getOperand(7), SE.getAddExpr(Sum)); - Sum.clear(); -#endif - - Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 70), A[4], B[4])); - EXPECT_EQ(Product->getOperand(8), SE.getAddExpr(Sum)); -} - TEST_F(ScalarEvolutionsTest, SimplifiedPHI) { FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context), std::vector(), false); diff --git a/unittests/Transforms/Scalar/LoopPassManagerTest.cpp b/unittests/Transforms/Scalar/LoopPassManagerTest.cpp index 0e5780ebec4..31b3ceac541 100644 --- a/unittests/Transforms/Scalar/LoopPassManagerTest.cpp +++ b/unittests/Transforms/Scalar/LoopPassManagerTest.cpp @@ -1374,9 +1374,8 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { // to isolate ourselves from the rest of LLVM and for simplicity. Here we can // egregiously cheat based on knowledge of the test case. For example, we // have no PHI nodes and there is always a single i-dom. - auto RemoveLoop = [](Loop &L, BasicBlock &IDomBB, - LoopStandardAnalysisResults &AR, - LPMUpdater &Updater) { + auto EraseLoop = [](Loop &L, BasicBlock &IDomBB, + LoopStandardAnalysisResults &AR, LPMUpdater &Updater) { assert(L.empty() && "Can only delete leaf loops with this routine!"); SmallVector LoopBBs(L.block_begin(), L.block_end()); Updater.markLoopAsDeleted(L); @@ -1394,10 +1393,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { for (BasicBlock *LoopBB : LoopBBs) LoopBB->eraseFromParent(); - if (Loop *ParentL = L.getParentLoop()) - return ParentL->removeChildLoop(find(*ParentL, &L)); - - return AR.LI.removeLoop(find(AR.LI, &L)); + AR.LI.markAsErased(&L); }; // Build up the pass managers. @@ -1442,7 +1438,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { LoopStandardAnalysisResults &AR, LPMUpdater &Updater) { Loop *ParentL = L.getParentLoop(); AR.SE.forgetLoop(&L); - delete RemoveLoop(L, Loop01PHBB, AR, Updater); + EraseLoop(L, Loop01PHBB, AR, Updater); ParentL->verifyLoop(); return PreservedAnalyses::all(); })); @@ -1469,10 +1465,8 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { .WillRepeatedly(Invoke(getLoopAnalysisResult)); // Run the loop pipeline again. This time we delete the last loop, which - // contains a nested loop within it, and we reuse its inner loop object to - // insert a new loop into the nest. This makes sure that we don't reuse - // cached analysis results for loop objects when removed just because their - // pointers match, and that we can handle nested loop deletion. + // contains a nested loop within it and insert a new loop into the nest. This + // makes sure we can handle nested loop deletion. AddLoopPipelineAndVerificationPasses(); EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _)) .Times(3) @@ -1489,16 +1483,16 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { .WillOnce( Invoke([&](Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR, LPMUpdater &Updater) { - // Remove the inner loop first but retain it to reuse later. AR.SE.forgetLoop(*L.begin()); - auto *OldL = RemoveLoop(**L.begin(), Loop020PHBB, AR, Updater); + EraseLoop(**L.begin(), Loop020PHBB, AR, Updater); auto *ParentL = L.getParentLoop(); AR.SE.forgetLoop(&L); - delete RemoveLoop(L, Loop02PHBB, AR, Updater); + EraseLoop(L, Loop02PHBB, AR, Updater); - // Now insert a new sibling loop, reusing a loop pointer. - ParentL->addChildLoop(OldL); + // Now insert a new sibling loop. + auto *NewSibling = new Loop; + ParentL->addChildLoop(NewSibling); NewLoop03PHBB = BasicBlock::Create(Context, "loop.0.3.ph", &F, &Loop0LatchBB); auto *NewLoop03BB = @@ -1515,10 +1509,10 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { AR.DT[NewLoop03BB]); AR.DT.verifyDomTree(); ParentL->addBasicBlockToLoop(NewLoop03PHBB, AR.LI); - OldL->addBasicBlockToLoop(NewLoop03BB, AR.LI); - OldL->verifyLoop(); + NewSibling->addBasicBlockToLoop(NewLoop03BB, AR.LI); + NewSibling->verifyLoop(); ParentL->verifyLoop(); - Updater.addSiblingLoops({OldL}); + Updater.addSiblingLoops({NewSibling}); return PreservedAnalyses::all(); })); @@ -1550,7 +1544,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { Invoke([&](Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR, LPMUpdater &Updater) { AR.SE.forgetLoop(&L); - delete RemoveLoop(L, Loop00PHBB, AR, Updater); + EraseLoop(L, Loop00PHBB, AR, Updater); return PreservedAnalyses::all(); })); @@ -1561,7 +1555,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { Invoke([&](Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR, LPMUpdater &Updater) { AR.SE.forgetLoop(&L); - delete RemoveLoop(L, *NewLoop03PHBB, AR, Updater); + EraseLoop(L, *NewLoop03PHBB, AR, Updater); return PreservedAnalyses::all(); })); @@ -1572,7 +1566,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) { Invoke([&](Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR, LPMUpdater &Updater) { AR.SE.forgetLoop(&L); - delete RemoveLoop(L, EntryBB, AR, Updater); + EraseLoop(L, EntryBB, AR, Updater); return PreservedAnalyses::all(); }));