[PATCH] D132199: [LoopPassManager] Ensure to construct loop nests with the outermost loop

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopPassManager.cpp:92-93
+    // outermost loop.
+    while (Loop *Parent = L->getParentLoop())
+      L = Parent;
+
----------------
aeubanks wrote:
> congzhe wrote:
> > aeubanks wrote:
> > > congzhe wrote:
> > > > aeubanks wrote:
> > > > > this feels like a hack and might hide future issues around accidentally swapping loops around
> > > > > 
> > > > > augmenting `LPMUpdater` to have a method to set the loopnest and having loop-interchange use that feels like a more proper and explicit solution
> > > > > 
> > > > > thoughts?
> > > > Thanks for commenting! Following your comments I've updated the patch such that if "LoopNestAnalysis" is not preserved by a pass (which essentially results in `IsLoopNestPtrValid=false`), we re-construct the loop nest based on the current outermost loop. IMHO this logic is the same as your proposal that if a pass invalidates the loopnest, we'll set the loopnest again.
> > > first, the existing code seems to be overly complicated, https://reviews.llvm.org/D132581 cleans up the loop nest analysis code
> > > 
> > > but the bigger thing is that this still seems too magical. the ideology behind the loop pass manager (and the CGSCC pass manager) is that passes should explicitly tell the pass manager what happens to loops. changing `L` throughout the execution isn't really intended, we should be bailing out and letting the function->loop adaptor visit whatever we need to visit next via `LPMUpdater::Worklist`
> > > 
> > > IIUC, loop-interchange can arbitrarily change the nesting of a loop nest. the question is, when that happens, what should the loop pass manager do? I'd imagine that if loop-interchange makes a change, we'd want to bail out after it's done, then completely revisit all the loops in the loop nest inside-out to optimize them in their new nesting. but I'd like to hear your thoughts on this (and if I understand loop-interchange correctly).
> > > 
> > > I had something like https://reviews.llvm.org/D132599 in mind (currently it only revisits the new outermost loop, but something along those lines API-wise), but it seems to still hang on your repro. but that seems like an issue with loop-interchange not being idempotent, i.e. if you keep running it on a loop, it keeps generating IR which seems bad
> > Thanks for your input! I applied D132581 and D132599 and did some quick debugging. The reason that opt keeps generating IR is that we add the current outermost loop to the `worklist` via LPMUpdater at the end of loop interchange, so the `worklist` is never empty and we keep poping loops from it and run optimizations on it forever. The behavior occurs for both the new and legacy pass manager.
> > 
> > Regarding loop interchange behavior: under the new pass manager it is a loopnest pass, so we are not concerned about the inner or middle loops at all from loop pass manager's perspective. We won't visit all the loops in the loop nest inside-out but we only visit the outermost loops and construct loop nests based on outermost loops. It is my understanding that after the loop interchange pass makes a change (which can be not one but multiple interchanges running one iteration of the pass), we would need to construct the loop nest again based on the current outermost loop, before running the next loop interchange pass. I'd appreciate it if you could let me know if it sounds clear to you.
> D132599 only adds the loop nest back to the worklist if it made a change. It sounds like if you keep running loop-interchange on a loop nest, it can keep optimizing it forever, rather than eventually stopping. That seems bad.
> 
> In terms of visiting loops, I understand that that this is a loop nest pass and only runs on top level loops. But my question is, given that there are other loop passes that aren't loop nest passes in the same LPM, if we have `loop-pass-A,loop-interchange,loop-pass-B` and loop-interchange makes a change (say on `L1(L2)` where L1 is the outer loop, L2 is the inner loop -> `L2(L1)`), how should the LPM deal with the new loop nest? Should it revisit everything starting from the inner-most loop (e.g. the LPM started with `L1(L2)`, visited L2 with loop-pass-A and loop-pass-B, then ran loop-pass-A on L1 and loop-interchange on the whole loop nest, interchanging the loops, now should it restart all the loop passes on `L2(L1)` starting with L1, then L2, because the structure changed)?
Thanks for this. The reason why loop interchange keeps optimizing the loopnest is that loop interchange can interchange loops either to achieve better locality, or to expose more vectorization opportunities. Following your terminology above, it may happen that for a loopnest L1(L2), loop interchange will interchange it to achieve better locality, resulting in L2(L1). Now if we run another loop interchange pass on it, it may still interchange the loopnest, resulting in L1(L2). This time the pass interchanges the loop because it might expose more vectorization opportunities. This behavior can go on and on.

In terms of visiting loops, I think what the pass manager does currently is as follows, which seems to be appropriate. With your example above, the LPM started with L1(L2), visited L2 with loop-pass-A and loop-pass-B, then ran loop-pass-A on L1 and loop-interchange on the whole loop nest, interchanging the loops which results in L2(L1).  Then it would ran run loop-pass-B on L1. It looks appropriate because L1 is the one that we want to optimize with loop-pass-B at this point, even it becase the inner loop after interchange. Nevertheless, we can always "restart all the loop passes on L2(L1) starting with L1, then L2, because the structure changed". The potential drawback is that it causes compile time increase, as well as the infinite run problem we described in our first paragraph.

I do get your point that we may not want to change `L` in this function `runWithLoopNestPasses()`. I've provided another solution which does not change `L` but keeps a pointer to the outermost loop. It is based on your patch D132581. Whenever we need to run loopnest passes, we contruct the loopnest based on the outermost loop. The interface with the LPM::Updater (`U.isLoopNestChanged()` and `U.markLoopNestChanged()`) may not be necessary though. It suffices to only add the while loop `while (auto *PL = OuterMostLoop->getParentLoop())` after `PassPA = runSinglePass(LN, Pass, AM, AR, U, PI)`.

I'd appreciate it if you could let me know your comments on the most recent version. Thanks a lot.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132199/new/

https://reviews.llvm.org/D132199