[PATCH] D16257: PM: Implement a basic loop pass manager

[Justin Bogner via llvm-commits]

Chandler Carruth <chandlerc at gmail.com> writes:
> chandlerc added a comment.
>
> OMG, the delaays reviewing. So sorry.
>
> Most of this is excellent. I think the tricky question is around
> handling the mutation of the loop nest correctly. I wonder if it'd be
> worthwhile to just rip handling of that out entirely in order to land
> the rest and then support mutation as a follow-up patch so we can look
> at the alternatives there without having so much boiler plate hanging
> in a patch?

Sure. I've gone ahead an committed a patch that puts the infrastructure
in place in r261831, taking into account your comments below.

>
> ================
> Comment at: include/llvm/Analysis/LoopInfo.h:473-478
> @@ +472,8 @@
> +  StringRef getName() const {
> +    // TODO: Is the name of the header block sufficient, or should we have
> +    // something like "<function> loop <header block>"?
> +    if (BasicBlock *Header = getHeader())
> +      if (Header->hasName())
> +        return Header->getName();
> +    return "<unnamed loop>";
> +  }
> ----------------
> I think this is fine, the caller can dig out more info as needed.

SGTM.

> ================
> Comment at: include/llvm/Analysis/LoopPassManager.h:41-44
> @@ +40,6 @@
> +
> +template <> class IRUnitTraits<Loop> {
> +public:
> +  static bool hasBeenInvalidated(Loop &IR) { return IR.isInvalid(); }
> +};
> +
> ----------------
> This trait isn't really commented, but I see what you're using it for
> (at least I think so -- to stop the LoopPassManager when the current
> loop gets deleted?).
>
> That technique feels a bit clumsy to me. What do you think about
> instead expanding the LoopPassManager to just be its own beast and
> have loop-specific logic where it halts the passes if the loop itself
> goes away, and logs helpfully? Maybe we could sink some of the
> boilerplate into a base class so it is reasonably easy to define a
> custom pass manager here?

I've dropped this for now.

The main issue I have with breaking this out into a separate
implementation is mostly the code duplication. A base class *might*
help, but it could pretty easily be just as awkward though, with a
virtual method hook instead. It's probably best just to try both and
see.

Is it possible for CGSCC (or something similar) to destroy an element
that it's just worked on in a similar way to loops being deleted? If so
I think something like what I've done here could be generalized pretty
sanely. If not, this might be over-abstracting.

> ================
> Comment at: include/llvm/Analysis/LoopPassManager.h:214-218
> @@ +213,7 @@
> +
> +  void enqueueLoop(Loop *L, std::deque<Loop *> &LQ) {
> +    LQ.push_back(L);
> +    for (auto *NestedLoop : reverse(*L))
> +      enqueueLoop(NestedLoop, LQ);
> +  }
> +
> ----------------
> Why not a worklist rather than explicit recursion? Either way, either
> inline the code or if still recursive use a lambda rather than a
> helper function?

Sure. I've done this.

> ================
> Comment at: include/llvm/Analysis/LoopPassManager.h:232
> @@ +231,3 @@
> +
> +    std::deque<Loop *> LQ;
> +    for (auto *L : reverse(LI))
> ----------------
> Why a deque? It looks like we just push_back, so a vector (or
> smallvector) would seem more appropriate?

I think this was an artifact of some other approaches I tried in terms
of mutating the loop nest as we go. I've switched to a SmallVector.

> ================
> Comment at: include/llvm/Analysis/LoopPassManager.h:233
> @@ +232,3 @@
> +    std::deque<Loop *> LQ;
> +    for (auto *L : reverse(LI))
> +      enqueueLoop(L, LQ);
> ----------------
> Some comment about the reverse?

Yep. I've added commentary to explain that this does reverse post-order
by building a stack in pre-order then iterating over it in reverse.

> ================
> Comment at: include/llvm/Analysis/LoopPassManager.h:236-237
> @@ +235,4 @@
> +
> +    for (auto *L : reverse(LQ)) {
> +      PreservedAnalyses PassPA = Pass.run(*L, LAM);
> +
> ----------------
> It's a bit odd that you don't directly handle updates to the loop
> structure here. I feel like you could by potentially avoiding the
> queue above entirely and directly sinking this into the recursive walk
> over the LoopInfo so that as passes update LoopInfo, in turn your
> traversal is kept up-to-date.

As long as we only support removing loops and not adding them, the
traversal just takes care of all of that and there really is nothing to
do here. This will fall apart for LoopUnswitch, or anything we add later
that wants to duplicate loops, so you're probably right that we want to
do something more explicit. Anyways, that's all for a followup.

> As part of that, maybe check that passes preserve LoopInfo here?

LoopInfo's a FunctionAnalysis and these are loop passes - they couldn't
invalidate it if they wanted to. It's probably worth writing some sort
of verifier that it's still correct here and asserting on that though.
Ideas on how to go about that are welcome.

>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D16257