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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopPassManager.cpp:92-93
+    // outermost loop.
+    while (Loop *Parent = L->getParentLoop())
+      L = Parent;
+
----------------
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)?


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

https://reviews.llvm.org/D132199