squiid/llvm
1
0
Fork 0
Code Issues Pull requests Activity

[SimpleLoopUnswitch] Inform pass manager when child loops are deleted

Browse source 
As part of the nontrivial unswitching we could end up removing child
loops. This patch add a notification to the pass manager when
that happens (using the markLoopAsDeleted callback).

Without this there could be stale LoopAccessAnalysis results cached
in the analysis manager. Those analysis results are cached based on
a Loop* as key. Since the BumpPtrAllocator used to allocate
Loop objects could be resetted between different runs of for
example the loop-distribute pass (running on different functions),
a new Loop object could be created using the same Loop pointer.
And then when requiring the LoopAccessAnalysis for the loop we
got the stale (corrupt) result from the destroyed loop.

Reviewed By: aeubanks

Differential Revision: https://reviews.llvm.org/D109257
This commit is contained in:
Bjorn Pettersson 2021-09-03 20:50:33 +02:00
parent 59c954f76a
commit 0f0344dd1e
2 changed files with 101 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

42
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
View file

@ -1587,10 +1587,12 @@ deleteDeadClonedBlocks(Loop &L, ArrayRef<BasicBlock *> ExitBlocks,
BB->eraseFromParent();
}
static void deleteDeadBlocksFromLoop(Loop &L,
SmallVectorImpl<BasicBlock *> &ExitBlocks,
DominatorTree &DT, LoopInfo &LI,
MemorySSAUpdater *MSSAU) {
static void
deleteDeadBlocksFromLoop(Loop &L,
SmallVectorImpl<BasicBlock *> &ExitBlocks,
DominatorTree &DT, LoopInfo &LI,
MemorySSAUpdater *MSSAU,
function_ref<void(Loop &, StringRef)> DestroyLoopCB) {
// Find all the dead blocks tied to this loop, and remove them from their
// successors.
SmallSetVector<BasicBlock *, 8> DeadBlockSet;
@ -1640,6 +1642,7 @@ static void deleteDeadBlocksFromLoop(Loop &L,
}) &&
"If the child loop header is dead all blocks in the child loop must "
"be dead as well!");
DestroyLoopCB(*ChildL, ChildL->getName());
LI.destroy(ChildL);
return true;
});
@ -1980,6 +1983,8 @@ static bool rebuildLoopAfterUnswitch(Loop &L, ArrayRef<BasicBlock *> ExitBlocks,
ParentL->removeChildLoop(llvm::find(*ParentL, &L));
else
LI.removeLoop(llvm::find(LI, &L));
// markLoopAsDeleted for L should be triggered by the caller (it is typically
// done by using the UnswitchCB callback).
LI.destroy(&L);
return false;
}
@ -2019,7 +2024,8 @@ static void unswitchNontrivialInvariants(
SmallVectorImpl<BasicBlock *> &ExitBlocks, IVConditionInfo &PartialIVInfo,
DominatorTree &DT, LoopInfo &LI, AssumptionCache &AC,
function_ref<void(bool, bool, ArrayRef<Loop *>)> UnswitchCB,
ScalarEvolution *SE, MemorySSAUpdater *MSSAU) {
ScalarEvolution *SE, MemorySSAUpdater *MSSAU,
function_ref<void(Loop &, StringRef)> DestroyLoopCB) {
auto *ParentBB = TI.getParent();
BranchInst *BI = dyn_cast<BranchInst>(&TI);
SwitchInst *SI = BI ? nullptr : cast<SwitchInst>(&TI);
@ -2319,7 +2325,7 @@ static void unswitchNontrivialInvariants(
// Now that our cloned loops have been built, we can update the original loop.
// First we delete the dead blocks from it and then we rebuild the loop
// structure taking these deletions into account.
deleteDeadBlocksFromLoop(L, ExitBlocks, DT, LI, MSSAU);
deleteDeadBlocksFromLoop(L, ExitBlocks, DT, LI, MSSAU, DestroyLoopCB);
if (MSSAU && VerifyMemorySSA)
MSSAU->getMemorySSA()->verifyMemorySSA();
@ -2670,7 +2676,8 @@ static bool unswitchBestCondition(
Loop &L, DominatorTree &DT, LoopInfo &LI, AssumptionCache &AC,
AAResults &AA, TargetTransformInfo &TTI,
function_ref<void(bool, bool, ArrayRef<Loop *>)> UnswitchCB,
ScalarEvolution *SE, MemorySSAUpdater *MSSAU) {
ScalarEvolution *SE, MemorySSAUpdater *MSSAU,
function_ref<void(Loop &, StringRef)> DestroyLoopCB) {
// Collect all invariant conditions within this loop (as opposed to an inner
// loop which would be handled when visiting that inner loop).
SmallVector<std::pair<Instruction *, TinyPtrVector<Value *>>, 4>
@ -2958,7 +2965,7 @@ static bool unswitchBestCondition(
<< "\n");
unswitchNontrivialInvariants(L, *BestUnswitchTI, BestUnswitchInvariants,
ExitBlocks, PartialIVInfo, DT, LI, AC,
UnswitchCB, SE, MSSAU);
UnswitchCB, SE, MSSAU, DestroyLoopCB);
return true;
}
@ -2988,7 +2995,8 @@ unswitchLoop(Loop &L, DominatorTree &DT, LoopInfo &LI, AssumptionCache &AC,
AAResults &AA, TargetTransformInfo &TTI, bool Trivial,
bool NonTrivial,
function_ref<void(bool, bool, ArrayRef<Loop *>)> UnswitchCB,
ScalarEvolution *SE, MemorySSAUpdater *MSSAU) {
ScalarEvolution *SE, MemorySSAUpdater *MSSAU,
function_ref<void(Loop &, StringRef)> DestroyLoopCB) {
assert(L.isRecursivelyLCSSAForm(DT, LI) &&
"Loops must be in LCSSA form before unswitching.");
@ -3036,7 +3044,8 @@ unswitchLoop(Loop &L, DominatorTree &DT, LoopInfo &LI, AssumptionCache &AC,
// Try to unswitch the best invariant condition. We prefer this full unswitch to
// a partial unswitch when possible below the threshold.
if (unswitchBestCondition(L, DT, LI, AC, AA, TTI, UnswitchCB, SE, MSSAU))
if (unswitchBestCondition(L, DT, LI, AC, AA, TTI, UnswitchCB, SE, MSSAU,
DestroyLoopCB))
return true;
// No other opportunities to unswitch.
@ -3083,6 +3092,10 @@ PreservedAnalyses SimpleLoopUnswitchPass::run(Loop &L, LoopAnalysisManager &AM,
U.markLoopAsDeleted(L, LoopName);
};
auto DestroyLoopCB = [&U](Loop &L, StringRef Name) {
U.markLoopAsDeleted(L, Name);
};
Optional<MemorySSAUpdater> MSSAU;
if (AR.MSSA) {
MSSAU = MemorySSAUpdater(AR.MSSA);
@ -3091,7 +3104,8 @@ PreservedAnalyses SimpleLoopUnswitchPass::run(Loop &L, LoopAnalysisManager &AM,
}
if (!unswitchLoop(L, AR.DT, AR.LI, AR.AC, AR.AA, AR.TTI, Trivial, NonTrivial,
UnswitchCB, &AR.SE,
MSSAU.hasValue() ? MSSAU.getPointer() : nullptr))
MSSAU.hasValue() ? MSSAU.getPointer() : nullptr,
DestroyLoopCB))
return PreservedAnalyses::all();
if (AR.MSSA && VerifyMemorySSA)
@ -3173,11 +3187,15 @@ bool SimpleLoopUnswitchLegacyPass::runOnLoop(Loop *L, LPPassManager &LPM) {
LPM.markLoopAsDeleted(*L);
};
auto DestroyLoopCB = [&LPM](Loop &L, StringRef /* Name */) {
LPM.markLoopAsDeleted(L);
};
if (VerifyMemorySSA)
MSSA->verifyMemorySSA();
bool Changed = unswitchLoop(*L, DT, LI, AC, AA, TTI, true, NonTrivial,
UnswitchCB, SE, &MSSAU);
UnswitchCB, SE, &MSSAU, DestroyLoopCB);
if (VerifyMemorySSA)
MSSA->verifyMemorySSA();

71
llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll Normal file
View file

@ -0,0 +1,71 @@
; RUN: opt < %s -enable-loop-distribute -passes='loop-distribute,loop-mssa(simple-loop-unswitch<nontrivial>),loop-distribute' -o /dev/null -S -debug-pass-manager=verbose 2>&1 | FileCheck %s
; Running loop-distribute will result in LoopAccessAnalysis being required and
; cached in the LoopAnalysisManagerFunctionProxy.
;
; CHECK: Running analysis: LoopAccessAnalysis on Loop at depth 2 containing: %loop_a_inner<header><latch><exiting>
; Then simple-loop-unswitch is removing/replacing some loops (resulting in
; Loop objects used as key in the analyses cache is destroyed). So here we
; want to see that any analysis results cached on the destroyed loop is
; cleared. A special case here is that loop_a_inner is destroyed when
; unswitching the parent loop.
;
; The bug solved and verified by this test case was related to the
; SimpleLoopUnswitch not marking the Loop as removed, so we missed clearing
; the analysis caches.
;
; CHECK: Running pass: SimpleLoopUnswitchPass on Loop at depth 1 containing: %loop_begin<header>,%loop_b,%loop_b_inner,%loop_b_inner_exit,%loop_a,%loop_a_inner,%loop_a_inner_exit,%latch<latch><exiting>
; CHECK-NEXT: Clearing all analysis results for: loop_a_inner
; When running loop-distribute the second time we can see that loop_a_inner
; isn't analysed because the loop no longer exists (instead we find a new loop,
; loop_a_inner.us). This kind of verifies that it was correct to remove the
; loop_a_inner related analysis above.
;
; CHECK: Running analysis: LoopAccessAnalysis on Loop at depth 2 containing: %loop_a_inner.us<header><latch><exiting>
define i32 @test6(i1* %ptr, i1 %cond1, i32* %a.ptr, i32* %b.ptr) {
entry:
br label %loop_begin
loop_begin:
%v = load i1, i1* %ptr
br i1 %cond1, label %loop_a, label %loop_b
loop_a:
br label %loop_a_inner
loop_a_inner:
%va = load i1, i1* %ptr
%a = load i32, i32* %a.ptr
br i1 %va, label %loop_a_inner, label %loop_a_inner_exit
loop_a_inner_exit:
%a.lcssa = phi i32 [ %a, %loop_a_inner ]
br label %latch
loop_b:
br label %loop_b_inner
loop_b_inner:
%vb = load i1, i1* %ptr
%b = load i32, i32* %b.ptr
br i1 %vb, label %loop_b_inner, label %loop_b_inner_exit
loop_b_inner_exit:
%b.lcssa = phi i32 [ %b, %loop_b_inner ]
br label %latch
latch:
%ab.phi = phi i32 [ %a.lcssa, %loop_a_inner_exit ], [ %b.lcssa, %loop_b_inner_exit ]
br i1 %v, label %loop_begin, label %loop_exit
loop_exit:
%ab.lcssa = phi i32 [ %ab.phi, %latch ]
ret i32 %ab.lcssa
}