Skip to content

Commit 76ab232

Browse files
sanjoysanjoy
committedSep 19, 2017
[LoopInfo] Make LoopBase and Loop destructors non-public
Summary: See comment for why I think this is a good idea. This change also: - Removes an SCEV test case. The SCEV test was not testing anything useful (most of it was `#if 0` ed out) and it would need to be updated to deal with a private ~Loop::Loop. - Updates the loop pass manager test case to deal with a private ~Loop::Loop. - Renames markAsRemoved to markAsErased to contrast with removeLoop, via the usual remove vs. erase idiom we already have for instructions and basic blocks. Reviewers: chandlerc Subscribers: mehdi_amini, mcrosier, llvm-commits Differential Revision: https://reviews.llvm.org/D37996 llvm-svn: 313695
·
llvmorg-21-initllvmorg-6.0.0-rc1
1 parent fbfaec7 commit 76ab232

File tree

7 files changed

+40
-175
lines changed

7 files changed

+40
-175
lines changed
 

‎llvm/include/llvm/Analysis/LoopInfo.h

+17-5
Original file line numberDiff line numberDiff line change
@@ -85,10 +85,6 @@ class LoopBase {
8585
public:
8686
/// This creates an empty loop.
8787
LoopBase() : ParentLoop(nullptr) {}
88-
~LoopBase() {
89-
for (size_t i = 0, e = SubLoops.size(); i != e; ++i)
90-
delete SubLoops[i];
91-
}
9288

9389
/// Return the nesting level of this loop. An outer-most loop has depth 1,
9490
/// for consistency with loop depth values used for basic blocks, where depth
@@ -343,6 +339,20 @@ class LoopBase {
343339
Blocks.push_back(BB);
344340
DenseBlockSet.insert(BB);
345341
}
342+
343+
// Since loop passes like SCEV are allowed to key analysis results off of
344+
// `Loop` pointers, we cannot re-use pointers within a loop pass manager.
345+
// This means loop passes should not be `delete` ing `Loop` objects directly
346+
// (and risk a later `Loop` allocation re-using the address of a previous one)
347+
// but should be using LoopInfo::markAsRemoved, which keeps around the `Loop`
348+
// pointer till the end of the lifetime of the `LoopInfo` object.
349+
//
350+
// To make it easier to follow this rule, we mark the destructor as
351+
// non-public.
352+
~LoopBase() {
353+
for (auto *SubLoop : SubLoops)
354+
delete SubLoop;
355+
}
346356
};
347357

348358
template<class BlockT, class LoopT>
@@ -500,7 +510,9 @@ class Loop : public LoopBase<BasicBlock, Loop> {
500510

501511
private:
502512
friend class LoopInfoBase<BasicBlock, Loop>;
513+
friend class LoopBase<BasicBlock, Loop>;
503514
explicit Loop(BasicBlock *BB) : LoopBase<BasicBlock, Loop>(BB) {}
515+
~Loop() = default;
504516
};
505517

506518
//===----------------------------------------------------------------------===//
@@ -702,7 +714,7 @@ class LoopInfo : public LoopInfoBase<BasicBlock, Loop> {
702714
/// the loop forest and parent loops for each block so that \c L is no longer
703715
/// referenced, but does not actually delete \c L immediately. The pointer
704716
/// will remain valid until this LoopInfo's memory is released.
705-
void markAsRemoved(Loop *L);
717+
void markAsErased(Loop *L);
706718

707719
/// Returns true if replacing From with To everywhere is guaranteed to
708720
/// preserve LCSSA form.

‎llvm/lib/Analysis/LoopInfo.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -622,8 +622,8 @@ bool LoopInfo::invalidate(Function &F, const PreservedAnalyses &PA,
622622
PAC.preservedSet<CFGAnalyses>());
623623
}
624624

625-
void LoopInfo::markAsRemoved(Loop *Unloop) {
626-
assert(!Unloop->isInvalid() && "Loop has already been removed");
625+
void LoopInfo::markAsErased(Loop *Unloop) {
626+
assert(!Unloop->isInvalid() && "Loop has already been erased!");
627627
Unloop->invalidate();
628628
RemovedLoops.push_back(Unloop);
629629

‎llvm/lib/Transforms/IPO/LoopExtractor.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ bool LoopExtractor::runOnLoop(Loop *L, LPPassManager &) {
143143
Changed = true;
144144
// After extraction, the loop is replaced by a function call, so
145145
// we shouldn't try to run any more loop passes on it.
146-
LI.markAsRemoved(L);
146+
LI.markAsErased(L);
147147
}
148148
++NumExtracted;
149149
}

‎llvm/lib/Transforms/Scalar/LoopDeletion.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ static void deleteDeadLoop(Loop *L, DominatorTree &DT, ScalarEvolution &SE,
334334
LI.removeBlock(BB);
335335

336336
// The last step is to update LoopInfo now that we've eliminated this loop.
337-
LI.markAsRemoved(L);
337+
LI.markAsErased(L);
338338
}
339339

340340
PreservedAnalyses LoopDeletionPass::run(Loop &L, LoopAnalysisManager &AM,

‎llvm/lib/Transforms/Utils/LoopUnroll.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -816,7 +816,7 @@ bool llvm::UnrollLoop(Loop *L, unsigned Count, unsigned TripCount, bool Force,
816816
Loop *OuterL = L->getParentLoop();
817817
// Update LoopInfo if the loop is completely removed.
818818
if (CompletelyUnroll)
819-
LI->markAsRemoved(L);
819+
LI->markAsErased(L);
820820

821821
// After complete unrolling most of the blocks should be contained in OuterL.
822822
// 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,
841841
if (NeedToFixLCSSA) {
842842
// LCSSA must be performed on the outermost affected loop. The unrolled
843843
// loop's last loop latch is guaranteed to be in the outermost loop
844-
// after LoopInfo's been updated by markAsRemoved.
844+
// after LoopInfo's been updated by markAsErased.
845845
Loop *LatchLoop = LI->getLoopFor(Latches.back());
846846
Loop *FixLCSSALoop = OuterL;
847847
if (!FixLCSSALoop->contains(LatchLoop))

‎llvm/unittests/Analysis/ScalarEvolutionTest.cpp

-141
Original file line numberDiff line numberDiff line change
@@ -110,147 +110,6 @@ TEST_F(ScalarEvolutionsTest, SCEVUnknownRAUW) {
110110
EXPECT_EQ(cast<SCEVUnknown>(M2->getOperand(1))->getValue(), V0);
111111
}
112112

113-
TEST_F(ScalarEvolutionsTest, SCEVMultiplyAddRecs) {
114-
Type *Ty = Type::getInt32Ty(Context);
115-
SmallVector<Type *, 10> Types;
116-
Types.append(10, Ty);
117-
FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context), Types, false);
118-
Function *F = cast<Function>(M.getOrInsertFunction("f", FTy));
119-
BasicBlock *BB = BasicBlock::Create(Context, "entry", F);
120-
ReturnInst::Create(Context, nullptr, BB);
121-
122-
ScalarEvolution SE = buildSE(*F);
123-
124-
// It's possible to produce an empty loop through the default constructor,
125-
// but you can't add any blocks to it without a LoopInfo pass.
126-
Loop L;
127-
const_cast<std::vector<BasicBlock*>&>(L.getBlocks()).push_back(BB);
128-
129-
Function::arg_iterator AI = F->arg_begin();
130-
SmallVector<const SCEV *, 5> A;
131-
A.push_back(SE.getSCEV(&*AI++));
132-
A.push_back(SE.getSCEV(&*AI++));
133-
A.push_back(SE.getSCEV(&*AI++));
134-
A.push_back(SE.getSCEV(&*AI++));
135-
A.push_back(SE.getSCEV(&*AI++));
136-
const SCEV *A_rec = SE.getAddRecExpr(A, &L, SCEV::FlagAnyWrap);
137-
138-
SmallVector<const SCEV *, 5> B;
139-
B.push_back(SE.getSCEV(&*AI++));
140-
B.push_back(SE.getSCEV(&*AI++));
141-
B.push_back(SE.getSCEV(&*AI++));
142-
B.push_back(SE.getSCEV(&*AI++));
143-
B.push_back(SE.getSCEV(&*AI++));
144-
const SCEV *B_rec = SE.getAddRecExpr(B, &L, SCEV::FlagAnyWrap);
145-
146-
/* Spot check that we perform this transformation:
147-
{A0,+,A1,+,A2,+,A3,+,A4} * {B0,+,B1,+,B2,+,B3,+,B4} =
148-
{A0*B0,+,
149-
A1*B0 + A0*B1 + A1*B1,+,
150-
A2*B0 + 2A1*B1 + A0*B2 + 2A2*B1 + 2A1*B2 + A2*B2,+,
151-
A3*B0 + 3A2*B1 + 3A1*B2 + A0*B3 + 3A3*B1 + 6A2*B2 + 3A1*B3 + 3A3*B2 +
152-
3A2*B3 + A3*B3,+,
153-
A4*B0 + 4A3*B1 + 6A2*B2 + 4A1*B3 + A0*B4 + 4A4*B1 + 12A3*B2 + 12A2*B3 +
154-
4A1*B4 + 6A4*B2 + 12A3*B3 + 6A2*B4 + 4A4*B3 + 4A3*B4 + A4*B4,+,
155-
5A4*B1 + 10A3*B2 + 10A2*B3 + 5A1*B4 + 20A4*B2 + 30A3*B3 + 20A2*B4 +
156-
30A4*B3 + 30A3*B4 + 20A4*B4,+,
157-
15A4*B2 + 20A3*B3 + 15A2*B4 + 60A4*B3 + 60A3*B4 + 90A4*B4,+,
158-
35A4*B3 + 35A3*B4 + 140A4*B4,+,
159-
70A4*B4}
160-
*/
161-
162-
const SCEVAddRecExpr *Product =
163-
dyn_cast<SCEVAddRecExpr>(SE.getMulExpr(A_rec, B_rec));
164-
ASSERT_TRUE(Product);
165-
ASSERT_EQ(Product->getNumOperands(), 9u);
166-
167-
SmallVector<const SCEV *, 16> Sum;
168-
Sum.push_back(SE.getMulExpr(A[0], B[0]));
169-
EXPECT_EQ(Product->getOperand(0), SE.getAddExpr(Sum));
170-
Sum.clear();
171-
172-
// SCEV produces different an equal but different expression for these.
173-
// Re-enable when PR11052 is fixed.
174-
#if 0
175-
Sum.push_back(SE.getMulExpr(A[1], B[0]));
176-
Sum.push_back(SE.getMulExpr(A[0], B[1]));
177-
Sum.push_back(SE.getMulExpr(A[1], B[1]));
178-
EXPECT_EQ(Product->getOperand(1), SE.getAddExpr(Sum));
179-
Sum.clear();
180-
181-
Sum.push_back(SE.getMulExpr(A[2], B[0]));
182-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[1], B[1]));
183-
Sum.push_back(SE.getMulExpr(A[0], B[2]));
184-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[2], B[1]));
185-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 2), A[1], B[2]));
186-
Sum.push_back(SE.getMulExpr(A[2], B[2]));
187-
EXPECT_EQ(Product->getOperand(2), SE.getAddExpr(Sum));
188-
Sum.clear();
189-
190-
Sum.push_back(SE.getMulExpr(A[3], B[0]));
191-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[2], B[1]));
192-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[1], B[2]));
193-
Sum.push_back(SE.getMulExpr(A[0], B[3]));
194-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[3], B[1]));
195-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[2]));
196-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[1], B[3]));
197-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[3], B[2]));
198-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 3), A[2], B[3]));
199-
Sum.push_back(SE.getMulExpr(A[3], B[3]));
200-
EXPECT_EQ(Product->getOperand(3), SE.getAddExpr(Sum));
201-
Sum.clear();
202-
203-
Sum.push_back(SE.getMulExpr(A[4], B[0]));
204-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[3], B[1]));
205-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[2]));
206-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[1], B[3]));
207-
Sum.push_back(SE.getMulExpr(A[0], B[4]));
208-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[4], B[1]));
209-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[3], B[2]));
210-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[2], B[3]));
211-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[1], B[4]));
212-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[4], B[2]));
213-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 12), A[3], B[3]));
214-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 6), A[2], B[4]));
215-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[4], B[3]));
216-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 4), A[3], B[4]));
217-
Sum.push_back(SE.getMulExpr(A[4], B[4]));
218-
EXPECT_EQ(Product->getOperand(4), SE.getAddExpr(Sum));
219-
Sum.clear();
220-
221-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 5), A[4], B[1]));
222-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 10), A[3], B[2]));
223-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 10), A[2], B[3]));
224-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 5), A[1], B[4]));
225-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[4], B[2]));
226-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[3], B[3]));
227-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[2], B[4]));
228-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[4], B[3]));
229-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 30), A[3], B[4]));
230-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[4], B[4]));
231-
EXPECT_EQ(Product->getOperand(5), SE.getAddExpr(Sum));
232-
Sum.clear();
233-
234-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 15), A[4], B[2]));
235-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 20), A[3], B[3]));
236-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 15), A[2], B[4]));
237-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 60), A[4], B[3]));
238-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 60), A[3], B[4]));
239-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 90), A[4], B[4]));
240-
EXPECT_EQ(Product->getOperand(6), SE.getAddExpr(Sum));
241-
Sum.clear();
242-
243-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 35), A[4], B[3]));
244-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 35), A[3], B[4]));
245-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 140), A[4], B[4]));
246-
EXPECT_EQ(Product->getOperand(7), SE.getAddExpr(Sum));
247-
Sum.clear();
248-
#endif
249-
250-
Sum.push_back(SE.getMulExpr(SE.getConstant(Ty, 70), A[4], B[4]));
251-
EXPECT_EQ(Product->getOperand(8), SE.getAddExpr(Sum));
252-
}
253-
254113
TEST_F(ScalarEvolutionsTest, SimplifiedPHI) {
255114
FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context),
256115
std::vector<Type *>(), false);

‎llvm/unittests/Transforms/Scalar/LoopPassManagerTest.cpp

+17-23
Original file line numberDiff line numberDiff line change
@@ -1374,9 +1374,8 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
13741374
// to isolate ourselves from the rest of LLVM and for simplicity. Here we can
13751375
// egregiously cheat based on knowledge of the test case. For example, we
13761376
// have no PHI nodes and there is always a single i-dom.
1377-
auto RemoveLoop = [](Loop &L, BasicBlock &IDomBB,
1378-
LoopStandardAnalysisResults &AR,
1379-
LPMUpdater &Updater) {
1377+
auto EraseLoop = [](Loop &L, BasicBlock &IDomBB,
1378+
LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
13801379
assert(L.empty() && "Can only delete leaf loops with this routine!");
13811380
SmallVector<BasicBlock *, 4> LoopBBs(L.block_begin(), L.block_end());
13821381
Updater.markLoopAsDeleted(L);
@@ -1394,10 +1393,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
13941393
for (BasicBlock *LoopBB : LoopBBs)
13951394
LoopBB->eraseFromParent();
13961395

1397-
if (Loop *ParentL = L.getParentLoop())
1398-
return ParentL->removeChildLoop(find(*ParentL, &L));
1399-
1400-
return AR.LI.removeLoop(find(AR.LI, &L));
1396+
AR.LI.markAsErased(&L);
14011397
};
14021398

14031399
// Build up the pass managers.
@@ -1442,7 +1438,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
14421438
LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
14431439
Loop *ParentL = L.getParentLoop();
14441440
AR.SE.forgetLoop(&L);
1445-
delete RemoveLoop(L, Loop01PHBB, AR, Updater);
1441+
EraseLoop(L, Loop01PHBB, AR, Updater);
14461442
ParentL->verifyLoop();
14471443
return PreservedAnalyses::all();
14481444
}));
@@ -1469,10 +1465,8 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
14691465
.WillRepeatedly(Invoke(getLoopAnalysisResult));
14701466

14711467
// Run the loop pipeline again. This time we delete the last loop, which
1472-
// contains a nested loop within it, and we reuse its inner loop object to
1473-
// insert a new loop into the nest. This makes sure that we don't reuse
1474-
// cached analysis results for loop objects when removed just because their
1475-
// pointers match, and that we can handle nested loop deletion.
1468+
// contains a nested loop within it and insert a new loop into the nest. This
1469+
// makes sure we can handle nested loop deletion.
14761470
AddLoopPipelineAndVerificationPasses();
14771471
EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _))
14781472
.Times(3)
@@ -1489,16 +1483,16 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
14891483
.WillOnce(
14901484
Invoke([&](Loop &L, LoopAnalysisManager &AM,
14911485
LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
1492-
// Remove the inner loop first but retain it to reuse later.
14931486
AR.SE.forgetLoop(*L.begin());
1494-
auto *OldL = RemoveLoop(**L.begin(), Loop020PHBB, AR, Updater);
1487+
EraseLoop(**L.begin(), Loop020PHBB, AR, Updater);
14951488

14961489
auto *ParentL = L.getParentLoop();
14971490
AR.SE.forgetLoop(&L);
1498-
delete RemoveLoop(L, Loop02PHBB, AR, Updater);
1491+
EraseLoop(L, Loop02PHBB, AR, Updater);
14991492

1500-
// Now insert a new sibling loop, reusing a loop pointer.
1501-
ParentL->addChildLoop(OldL);
1493+
// Now insert a new sibling loop.
1494+
auto *NewSibling = new Loop;
1495+
ParentL->addChildLoop(NewSibling);
15021496
NewLoop03PHBB =
15031497
BasicBlock::Create(Context, "loop.0.3.ph", &F, &Loop0LatchBB);
15041498
auto *NewLoop03BB =
@@ -1515,10 +1509,10 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
15151509
AR.DT[NewLoop03BB]);
15161510
AR.DT.verifyDomTree();
15171511
ParentL->addBasicBlockToLoop(NewLoop03PHBB, AR.LI);
1518-
OldL->addBasicBlockToLoop(NewLoop03BB, AR.LI);
1519-
OldL->verifyLoop();
1512+
NewSibling->addBasicBlockToLoop(NewLoop03BB, AR.LI);
1513+
NewSibling->verifyLoop();
15201514
ParentL->verifyLoop();
1521-
Updater.addSiblingLoops({OldL});
1515+
Updater.addSiblingLoops({NewSibling});
15221516
return PreservedAnalyses::all();
15231517
}));
15241518

@@ -1550,7 +1544,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
15501544
Invoke([&](Loop &L, LoopAnalysisManager &AM,
15511545
LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
15521546
AR.SE.forgetLoop(&L);
1553-
delete RemoveLoop(L, Loop00PHBB, AR, Updater);
1547+
EraseLoop(L, Loop00PHBB, AR, Updater);
15541548
return PreservedAnalyses::all();
15551549
}));
15561550

@@ -1561,7 +1555,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
15611555
Invoke([&](Loop &L, LoopAnalysisManager &AM,
15621556
LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
15631557
AR.SE.forgetLoop(&L);
1564-
delete RemoveLoop(L, *NewLoop03PHBB, AR, Updater);
1558+
EraseLoop(L, *NewLoop03PHBB, AR, Updater);
15651559
return PreservedAnalyses::all();
15661560
}));
15671561

@@ -1572,7 +1566,7 @@ TEST_F(LoopPassManagerTest, LoopDeletion) {
15721566
Invoke([&](Loop &L, LoopAnalysisManager &AM,
15731567
LoopStandardAnalysisResults &AR, LPMUpdater &Updater) {
15741568
AR.SE.forgetLoop(&L);
1575-
delete RemoveLoop(L, EntryBB, AR, Updater);
1569+
EraseLoop(L, EntryBB, AR, Updater);
15761570
return PreservedAnalyses::all();
15771571
}));
15781572

0 commit comments

Comments
 (0)
Please sign in to comment.