[PATCH] D54023: [LoopSimplifyCFG] Delete dead in-loop blocks

[Anna Thomas via Phabricator via llvm-commits]

anna requested changes to this revision.
anna added a comment.
This revision now requires changes to proceed.

Pls add -verify-memoryssa to the RUN command as well, now that MSSA is getting updated.
Comment inline.



================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:240
+      LI.removeBlock(BB);
+      DeleteDeadBlock(BB, &DTU);
+      ++NumLoopBlocksDeleted;
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > anna wrote:
> > > So, this removes the dead block and updates DT.
> > > However, what about this case:
> > > ```
> > > deadblock:
> > >   br anotherblock:
> > > 
> > > anotherblock:
> > >   <no phis, so we're guaranteed there's exactly one pred, which is deadblock>
> > >    
> > > ```
> > > DeleteDeadBlock does not recursively remove dead blocks. It handles the case where: `dead block` is *one of the predecessors* of `anotherblock` - see `BasicBlock::removePredecessor`.
> > > 
> > > I don't think `DeadLoopBlocks` contains `anotherblock`, since it became dead/unreachable when `deadblock` is dead. 
> > > 
> > `DeadLoopBlocks` must contain it by construction, otherwise it's a bug.
> >  I'll add tests on that.
> Actually we already test this in tests `dead_block_propogate_test_branch_loop`. In this test, block `dead` is dead because its predecessor goes to it by false condition, and `dummy` is dead because `dead` is its only predecessor.
> 
> It is all handled by the algorithm that collects our block sets: it knows what blocks will be dead after *all* folding will be done. It knows it all in advance.
yes, I see that. thanks for the clarification.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:441
   // Eliminate unconditional branches by merging blocks into their predecessors.
   Changed |= mergeBlocksIntoPredecessors(L, DT, LI, MSSAU);
 
----------------
Shouldn't we check here that L is still alive before calling `mergeBlocksIntoPredecessors`?  When header is removed from the loop, the loop is removed from LI and no longer  a loop (line 248).
If the case of the current loop being deleted is not handled in this patch, pls update the patch summary to state that. 

... Ah, I see the `deleteCurrentLoop` early return at start of analyze(). 

Could you please add an assert like this before deleting the loop at line 248:
```
if (LI.isLoopHeader(BB)) {
    assert(L != LI.getLoopFor(BB) && "should not be current loop!");  
    LI.erase(LI.getLoopFor(BB));
}
```



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

https://reviews.llvm.org/D54023