Skip to content

Commit

Permalink
[PM/LoopUnswitch] Fix how the cloned loops are handled when updating …
Browse files Browse the repository at this point in the history 
…analyses.

Summary:
I noticed this issue because we didn't put the primary cloned loop into
the `NonChildClonedLoops` vector and so never iterated on it. Once
I fixed that, it made it clear why I had to do a really complicated and
unnecesasry dance when updating the loops to remain in canonical form --
I was unwittingly working around the fact that the primary cloned loop
wasn't in the expected list of cloned loops. Doh!

Now that we include it in this vector, we don't need to return it and we
can consolidate the update logic as we correctly have a single place
where it can be handled.

I've just added a test for the iteration order aspect as every time
I changed the update logic partially or incorrectly here, an existing
test failed and caught it so that seems well covered (which is also
evidenced by the extensive working around of this missing update).

Reviewers: asbirlea, sanjoy

Subscribers: mcrosier, hiraditya, llvm-commits

Differential Revision: https://reviews.llvm.org/D47647

llvm-svn: 333811
  • Loading branch information
@chandlerc
chandlerc committed Jun 2, 2018
1 parent db5781a commit 9281503
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 44 deletions.
75 changes: 31 additions & 44 deletions llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
View file
Expand Up @@ -792,9 +792,9 @@ static Loop *cloneLoopNest(Loop &OrigRootL, Loop *RootParentL,
/// original loop, multiple cloned sibling loops may be created. All of them
/// are returned so that the newly introduced loop nest roots can be
/// identified.
static Loop *buildClonedLoops(Loop &OrigL, ArrayRef<BasicBlock *> ExitBlocks,
const ValueToValueMapTy &VMap, LoopInfo &LI,
SmallVectorImpl<Loop *> &NonChildClonedLoops) {
static void buildClonedLoops(Loop &OrigL, ArrayRef<BasicBlock *> ExitBlocks,
const ValueToValueMapTy &VMap, LoopInfo &LI,
SmallVectorImpl<Loop *> &NonChildClonedLoops) {
Loop *ClonedL = nullptr;

auto *OrigPH = OrigL.getLoopPreheader();
Expand Down Expand Up @@ -887,6 +887,7 @@ static Loop *buildClonedLoops(Loop &OrigL, ArrayRef<BasicBlock *> ExitBlocks,
} else {
LI.addTopLevelLoop(ClonedL);
}
NonChildClonedLoops.push_back(ClonedL);

ClonedL->reserveBlocks(BlocksInClonedLoop.size());
// We don't want to just add the cloned loop blocks based on how we
Expand Down Expand Up @@ -1040,9 +1041,6 @@ static Loop *buildClonedLoops(Loop &OrigL, ArrayRef<BasicBlock *> ExitBlocks,
NonChildClonedLoops.push_back(cloneLoopNest(
*ChildL, ExitLoopMap.lookup(ClonedChildHeader), VMap, LI));
}

// Return the main cloned loop if any.
return ClonedL;
}

static void
Expand Down Expand Up @@ -1608,8 +1606,7 @@ static bool unswitchInvariantBranch(
// different from the original structure due to the simplified CFG. This also
// handles inserting all the cloned blocks into the correct loops.
SmallVector<Loop *, 4> NonChildClonedLoops;
Loop *ClonedL =
buildClonedLoops(L, ExitBlocks, VMap, LI, NonChildClonedLoops);
buildClonedLoops(L, ExitBlocks, VMap, LI, NonChildClonedLoops);

// Delete anything that was made dead in the original loop due to
// unswitching.
Expand Down Expand Up @@ -1638,7 +1635,7 @@ static bool unswitchInvariantBranch(
// also need to cover any intervening loops. We add all of these loops to
// a list and sort them by loop depth to achieve this without updating
// unnecessary loops.
auto UpdateLCSSA = [&](Loop &UpdateL) {
auto UpdateLoop = [&](Loop &UpdateL) {
#ifndef NDEBUG
UpdateL.verifyLoop();
for (Loop *ChildL : UpdateL) {
Expand All @@ -1647,51 +1644,41 @@ static bool unswitchInvariantBranch(
"Perturbed a child loop's LCSSA form!");
}
#endif
// First build LCSSA for this loop so that we can preserve it when
// forming dedicated exits. We don't want to perturb some other loop's
// LCSSA while doing that CFG edit.
formLCSSA(UpdateL, DT, &LI, nullptr);

// For loops reached by this loop's original exit blocks we may
// introduced new, non-dedicated exits. At least try to re-form dedicated
// exits for these loops. This may fail if they couldn't have dedicated
// exits to start with.
formDedicatedExitBlocks(&UpdateL, &DT, &LI, /*PreserveLCSSA*/ true);
};

// For non-child cloned loops and hoisted loops, we just need to update LCSSA
// and we can do it in any order as they don't nest relative to each other.
for (Loop *UpdatedL : llvm::concat<Loop *>(NonChildClonedLoops, HoistedLoops))
UpdateLCSSA(*UpdatedL);
//
// Also check if any of the loops we have updated have become top-level loops
// as that will necessitate widening the outer loop scope.
for (Loop *UpdatedL :
llvm::concat<Loop *>(NonChildClonedLoops, HoistedLoops)) {
UpdateLoop(*UpdatedL);
if (!UpdatedL->getParentLoop())
OuterExitL = nullptr;
}
if (IsStillLoop) {
UpdateLoop(L);
if (!L.getParentLoop())
OuterExitL = nullptr;
}

// If the original loop had exit blocks, walk up through the outer most loop
// of those exit blocks to update LCSSA and form updated dedicated exits.
if (OuterExitL != &L) {
SmallVector<Loop *, 4> OuterLoops;
// We start with the cloned loop and the current loop if they are loops and
// move toward OuterExitL. Also, if either the cloned loop or the current
// loop have become top level loops we need to walk all the way out.
if (ClonedL) {
OuterLoops.push_back(ClonedL);
if (!ClonedL->getParentLoop())
OuterExitL = nullptr;
}
if (IsStillLoop) {
OuterLoops.push_back(&L);
if (!L.getParentLoop())
OuterExitL = nullptr;
}
// Grab all of the enclosing loops now.
if (OuterExitL != &L)
for (Loop *OuterL = ParentL; OuterL != OuterExitL;
OuterL = OuterL->getParentLoop())
OuterLoops.push_back(OuterL);

// Finally, update our list of outer loops. This is nicely ordered to work
// inside-out.
for (Loop *OuterL : OuterLoops) {
// First build LCSSA for this loop so that we can preserve it when
// forming dedicated exits. We don't want to perturb some other loop's
// LCSSA while doing that CFG edit.
UpdateLCSSA(*OuterL);

// For loops reached by this loop's original exit blocks we may
// introduced new, non-dedicated exits. At least try to re-form dedicated
// exits for these loops. This may fail if they couldn't have dedicated
// exits to start with.
formDedicatedExitBlocks(OuterL, &DT, &LI, /*PreserveLCSSA*/ true);
}
}
UpdateLoop(*OuterL);

#ifndef NDEBUG
// Verify the entire loop structure to catch any incorrect updates before we
Expand Down
101 changes: 101 additions & 0 deletions llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch.ll
View file
Expand Up @@ -2562,3 +2562,104 @@ outer.latch:
exit:
ret void
}

; Non-trivial loop unswitching where there are two invariant conditions, but the
; second one is only in the cloned copy of the loop after unswitching.
define i32 @test24(i1* %ptr, i1 %cond1, i1 %cond2) {
; CHECK-LABEL: @test24(
entry:
br label %loop_begin
; CHECK-NEXT: entry:
; CHECK-NEXT: br i1 %cond1, label %entry.split.us, label %entry.split

loop_begin:
br i1 %cond1, label %loop_a, label %loop_b

loop_a:
br i1 %cond2, label %loop_a_a, label %loop_a_c
; The second unswitched condition.
;
; CHECK: entry.split.us:
; CHECK-NEXT: br i1 %cond2, label %entry.split.us.split.us, label %entry.split.us.split

loop_a_a:
call void @a()
br label %latch
; The 'loop_a_a' unswitched loop.
;
; CHECK: entry.split.us.split.us:
; CHECK-NEXT: br label %loop_begin.us.us
;
; CHECK: loop_begin.us.us:
; CHECK-NEXT: br label %loop_a.us.us
;
; CHECK: loop_a.us.us:
; CHECK-NEXT: br label %loop_a_a.us.us
;
; CHECK: loop_a_a.us.us:
; CHECK-NEXT: call void @a()
; CHECK-NEXT: br label %latch.us.us
;
; CHECK: latch.us.us:
; CHECK-NEXT: %[[V:.*]] = load i1, i1* %ptr
; CHECK-NEXT: br i1 %[[V]], label %loop_begin.us.us, label %loop_exit.split.us.split.us
;
; CHECK: loop_exit.split.us.split.us:
; CHECK-NEXT: br label %loop_exit.split

loop_a_c:
call void @c()
br label %latch
; The 'loop_a_c' unswitched loop.
;
; CHECK: entry.split.us.split:
; CHECK-NEXT: br label %loop_begin.us
;
; CHECK: loop_begin.us:
; CHECK-NEXT: br label %loop_a.us
;
; CHECK: loop_a.us:
; CHECK-NEXT: br label %loop_a_c.us
;
; CHECK: loop_a_c.us:
; CHECK-NEXT: call void @c()
; CHECK-NEXT: br label %latch
;
; CHECK: latch.us:
; CHECK-NEXT: %[[V:.*]] = load i1, i1* %ptr
; CHECK-NEXT: br i1 %[[V]], label %loop_begin.us, label %loop_exit.split.us.split
;
; CHECK: loop_exit.split.us.split:
; CHECK-NEXT: br label %loop_exit.split

loop_b:
call void @b()
br label %latch
; The 'loop_b' unswitched loop.
;
; CHECK: entry.split:
; CHECK-NEXT: br label %loop_begin
;
; CHECK: loop_begin:
; CHECK-NEXT: br label %loop_b
;
; CHECK: loop_b:
; CHECK-NEXT: call void @b()
; CHECK-NEXT: br label %latch
;
; CHECK: latch:
; CHECK-NEXT: %[[V:.*]] = load i1, i1* %ptr
; CHECK-NEXT: br i1 %[[V]], label %loop_begin, label %loop_exit.split
;
; CHECK: loop_exit.split:
; CHECK-NEXT: br label %loop_exit

latch:
%v = load i1, i1* %ptr
br i1 %v, label %loop_begin, label %loop_exit

loop_exit:
ret i32 0
; CHECK: loop_exit:
; CHECK-NEXT: ret
}

0 comments on commit 9281503

Please sign in to comment.