[PATCH] D111328: [LoopFlatten] Mark loop analyses as perserved

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:665
   SE->forgetLoop(FI.InnerLoop);
   LI->erase(FI.InnerLoop);
 
----------------
aeubanks wrote:
> nikic wrote:
> > aeubanks wrote:
> > > separately, this looks like it's not telling LPMUpdater that a loop has been deleted which is an issue
> > I think this is actually fine, because this is a LoopNest pass, in which case LPMUpdater only has to be informed about the removal of top-level loops.
> If we delete a Loop, then allocate another Loop at the same address, we may fetch an invalid cached analysis later on. `LPMUpdater::markLoopAsDeleted()` clears cached analyses for that Loop.
After looking a bit more closely, here's the problem:
 * For LoopNest passes, the loop pass adaptor will magically go into LoopNestMode if the LPM only contains loop nest passes.
 * In LoopNestMode, only the top level loops are visited and trying to mark a non-top-level loop as deleted will assert (https://github.com/llvm/llvm-project/blob/bd5befb55087199ee1c0c3e344847cd06a2ca839/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h#L284).
 * In non-LoopNestMode you do need to mark non-top-level loops for the reason you mentioned.

However, as the pass doesn't control whether it gets used in LoopNestMode or not, this means that either you are forbidden from invalidating other loops (only LoopNest passes scheduled) or you are required to invalidate other loops (non-LoopNest passes scheduled as well). Something seems broken here.

Here's my test patch: https://gist.github.com/nikic/1a65b8cb384bb7e00084279255b93e65 It will currently assert for standalone uses of `-loop-flatten` because those use LoopNestMode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111328