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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

In D132199#3740143 <https://reviews.llvm.org/D132199#3740143>, @Whitney wrote:

> LGTM other than the comment from Bardia.

Thanks for the review :)



================
Comment at: llvm/lib/Transforms/Scalar/LoopPassManager.cpp:71
                                        LPMUpdater &U) {
-  assert(L.isOutermost() &&
+  assert(L->isOutermost() &&
          "Loop-nest passes should only run on top-level loops.");
----------------
bmahjour wrote:
> can we copy the address of L at the start of the function, so we don't have to change the reference to a pointer in the function's interface?
Thanks Bardia for your comment! I've updated the patch according to your comment.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132199