[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:
> > > 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.


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

https://reviews.llvm.org/D132199