[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#3785586 <https://reviews.llvm.org/D132199#3785586>, @aeubanks wrote:

> Sorry, I'd meant to reply but didn't find time, I'll try to be more prompt about responding to this
>
> This is definitely better than the previous version.
>
> IMO it would still be best if continually rerunning a pass on some IR would reach a fixed point. For example, when loop unswitching happens, we revisit the current loop (which means restarting the entire loop pipeline), and if we end up creating a new loop, we also add that new loop to the worklist. There has been care taken to ensure that unswitching cannot run forever. If interchange did this, we'd be able to just revisit the entire loop nest again. For the interchange example, you say that loop-interchange may continually swap between `L1(L2)` and `L2(L1)`, one of them must be better than the other, why can't we converge on the better nesting? I don't quite understand the "locality vs vectorization" argument, there still must be one nesting that's ultimately optimal in the end. Then we can restart the pipeline on the loop nest, or just the loops that got swapped around.
>
> But if that doesn't make sense, then something along these lines seems ok. I think it might be worth revisiting exactly how loop nest passes work, but I haven't thought too hard about it. But then again, this seems like a loop-interchange-specific issue, not a loop nest issue.
>
> also you'll have to rebase again since my patch was reverted
> also seems like we've dropped the test?

Thank you very much for the comment! I thought about it more and it looks that `IsLoopNestPtrValid &= PassPA->getChecker<LoopNestAnalysis>().preserved()` already serves as the interaction of a loopnest pass with the LPMUpdater. Although it does not explicitly interact with the LPMUpdater itself using something like `LPMUpdater::isLoopNestChanged()`, the fact that `PA` does not preserve `LoopNestAnalysis` already indicates that the loop nest is changed. Therefore, I've now added the re-construction of loop nest under `if (!IsLoopNestPtrValid)`. I'd appreciate it if you could take a look.

Regarding reaching a fixed point for loop interchange: I fully agree with you that it would be idea if we could reach a fixed point instead of running forever. And yes there should be one nesting that's ultimately optimal in the end. It's just with the current cost analysis in loop interchange, sometimes it has not yet been made clear which nesting (`L1(L2)` versus `L2(L1)`, or `locality versus vectorization`) would be better. That is something that I think we can improve in the longer term.

I'm wondering if what I described above makes sense to you?


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

https://reviews.llvm.org/D132199