[PATCH] D57221: [LoopSimplifyCFG] Change logic of dead loops removal to avoid hitting asserts

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked an inline comment as done.
mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:408-409
+        Loop *DL = LI.getLoopFor(BB);
+        if (DL->getParentLoop()) {
+          for (auto *PL = DL->getParentLoop(); PL; PL = PL->getParentLoop())
+            for (auto *BB : DL->getBlocks())
----------------
uabelho wrote:
> mkazantsev wrote:
> > uabelho wrote:
> > > mkazantsev wrote:
> > > > uabelho wrote:
> > > > > Perhaps replace
> > > > > 
> > > > >         if (DL->getParentLoop()) {
> > > > >           for (auto *PL = DL->getParentLoop(); PL; PL = PL->getParentLoop())
> > > > > 
> > > > > with
> > > > > 
> > > > >         while (auto *PL = DL->getParentLoop()) {
> > > > > 
> > > > > ?
> > > > And loop infinitely? :) We need to change DL somehow, otherwise PL is always the same loop.
> > > Doesn't
> > >  DL->getParentLoop()->removeChildLoop(DL);
> > > disconnect DL from PL, so in the next loop round DL doesn't have the old PL as parent anymore?
> > The line `DL->getParentLoop()->removeChildLoop(DL);` is executed once *after* this loop. You seem to be misreading the code. :)
> Right. When I tried what I suggested I did
> 
> ```
>         while (auto *PL = DL->getParentLoop()) {
>           for (auto *BB : DL->getBlocks())
>             PL->removeBlockFromLoop(BB);
>           DL->getParentLoop()->removeChildLoop(DL);
>           LI.addTopLevelLoop(DL);
>         }
> ```
> with that all the testcases (including the reproducer in this patch) passes but perhaps it's stupid to call LI.addTopLevelLoop(DL) several times.
> But nvm, I just thought the
> 
> ```
> if (DL->getParentLoop()) {
>   for (auto *PL = DL->getParentLoop(); PL; PL = PL->getParentLoop())
> ```
> was cumbersome and could be simplified, I didn't mean to make a mess :)
I've seen that pattern across the code and think it's fine. Feel free to refactor it if you like. :)


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

https://reviews.llvm.org/D57221