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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:237
+                        << "\n");
+      if (LI.isLoopHeader(BB))
+        LI.erase(LI.getLoopFor(BB));
----------------
anna wrote:
> Looking at how DeadLoopBlocks get populated, these contain blocks within sub-loops as well.
> 
> So, there are 2 ways a subloop can become dead (either properly nested loop or a sibling loop). `BB` here belongs to a subloop and is:
> 1. header of that loop, which is the case you've handled here, i.e. that loop is removed from LI.
> 2. latch of that loop, which I don't see being handled. Once latch is removed, that subloop is no longer a loop either.
When collecting fold candidates, we *only* collect candidates that immediately belong to the current loop. Therefore, we cannot modify inner loop's backedge while processing outer loop, assuming that we should have handled it while handling the inner loop.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:240
+      LI.removeBlock(BB);
+      DeleteDeadBlock(BB, &DTU);
+      ++NumLoopBlocksDeleted;
----------------
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.


https://reviews.llvm.org/D54023