[PATCH] D14526: Don't recompute LCSSA after loop-unrolling when possible.

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Nov-09, at 20:54, Michael Zolotukhin <mzolotukhin at apple.com> wrote:
> 
> mzolotukhin created this revision.
> mzolotukhin added reviewers: chandlerc, hfinkel, dexonsmith, bogner, joker.eph.
> mzolotukhin added a subscriber: llvm-commits.
> 
> Currently we always recompute LCSSA for outer loops after unrolling an
> inner loop. That leads to compile time problem when we have big loop
> nests, and we can solve it by avoiding unnecessary work. For instance,
> if w eonly do partial unrolling, we don't break LCSSA, so we don't need
> to rebuild it. Also, if all exits from the inner loop are inside the
> enclosing loop, then complete unrolling won't break LCSSA either.
> 
> I replaced unconditional LCSSA recomputation with conditional recomputation +
> unconditional assert and added several tests, which were failing when I
> experimented with it.
> 
> Soon I plan to follow up with a similar patch for recalculation of dominators
> tree.
> 
> http://reviews.llvm.org/D14526
> 
> Files:
>  lib/Transforms/Utils/LoopUnroll.cpp
>  test/Transforms/LoopUnroll/rebuild_lcssa.ll
> 
> <D14526.39782.patch>

This makes sense to me, although I don't know the loop algorithms too well
so maybe someone else can confirm.  A few nitpicks below.

> Index: lib/Transforms/Utils/LoopUnroll.cpp
> ===================================================================
> --- lib/Transforms/Utils/LoopUnroll.cpp
> +++ lib/Transforms/Utils/LoopUnroll.cpp
> @@ -221,6 +221,16 @@
>  
>    // Are we eliminating the loop control altogether?
>    bool CompletelyUnroll = Count == TripCount;
> +  SmallVector<BasicBlock *, 4> ExitBlocks;
> +  L->getExitBlocks(ExitBlocks);
> +  Loop *ParentL = L->getParentLoop();
> +  bool AllExitsAreInsideParentLoop = true;
> +  for (auto BB : ExitBlocks) {

This would be clearer as `const BasicBlock *BB`, but should at least be
`const auto *BB` to make clear to readers that there are no copies going
on.

> +    if (LI->getLoopFor(BB) != ParentL) {
> +      AllExitsAreInsideParentLoop = false;
> +      break;
> +    }
> +  }
>  
>    // We assume a run-time trip count if the compiler cannot
>    // figure out the loop trip count and the unroll-runtime
> @@ -554,7 +564,11 @@
>          while (OuterL->getParentLoop() != LatchLoop)
>            OuterL = OuterL->getParentLoop();
>  
> -      formLCSSARecursively(*OuterL, *DT, LI, SE);
> +      if (CompletelyUnroll && !AllExitsAreInsideParentLoop)
> +        formLCSSARecursively(*OuterL, *DT, LI, SE);
> +
> +      assert(OuterL->isLCSSAForm(*DT) &&

Is this check cheap?  If not, it seems wasteful to do it when we've just
called `formLCSSARecursively()`.  Can you put this in an `else`, or do
an early continue inside the `if`?

> +             "Must have loops in LCSSA form to after loop-unroll.");

s/to after/after/

>      }
>    }
>  
>