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

> I think I'm ok with this sort of patch being a temporary workaround
> could you add a FIXME pointing to the discussion here?
>
> Using PreservedAnalyses to tell the loop pass manager that the loop nest structure has changed is still IMO an abuse of it. The whole point of PreservedAnalyses is that we determine whether or not some cached analysis for a given IR unit should be invalidated. The loop nest analysis shouldn't be modeled as a loop analysis since it's not at the same IR level (that's why https://reviews.llvm.org/D132581 is wrong, caused issues, and was reverted). This is essentially adding an extra bit to the pass's return value to tell the loop pass manager to change how it behaves, which is not what PreservedAnalyses is intended to be used for. But that's exactly why LPMUpdater exists, for this sort of thing. So I'd still like to go back to the LPMUpdater version.
>
> I also think there may be issues with loop analyses not being invalidated in regards to loop-interchange. If an inner loop has a cached analysis, right now I believe interchanging does not invalidate the (original) inner loop's analyses, but we may end up running a loop nest pass on it with its old analyses once it's the outer loop, even though the loop structure has changed. But that can be a separate patch.

Thanks for your clarification regarding `PreservedAnalyses` and `LPMUpdater`, I see your point. I've updated the patch again trying to incorporate LPMUpdater for this purpose. The reason that I dropped the use of LPMUpdater in my last patch is that, since D132581 <https://reviews.llvm.org/D132581> is reverted, it becomes a bit less straightforward to directly use LPMUpdater instead of "IsLoopNestPtrValid". Now I added the use of LPMUpdater back to the patch, and I added a FIXME that says we should not rely on PreservedAnalyses in the long run (I guess once D132581 <https://reviews.llvm.org/D132581> is re-landed we could remove "IsLoopNestPtrValid"?). I'd appreciate it if you could take a second look :)


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

https://reviews.llvm.org/D132199