This patch teaches LoopSimplifyCFG to delete loop blocks that have
become unreachable after terminator folding has been done.
Details
- Reviewers
escha anna skatkov fedor.sergeev reames - Commits
- rG347c58377291: Return "[LoopSimplifyCFG] Delete dead in-loop blocks"
rG0b1d069d64a3: [LoopSimplifyCFG] Delete dead in-loop blocks
rL350045: Return "[LoopSimplifyCFG] Delete dead in-loop blocks"
rL348457: [LoopSimplifyCFG] Delete dead in-loop blocks
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Transforms/Scalar/LoopSimplifyCFG.cpp | ||
---|---|---|
237 ↗ | (On Diff #174730) | 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:
|
240 ↗ | (On Diff #174730) | So, this removes the dead block and updates DT. 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. |
lib/Transforms/Scalar/LoopSimplifyCFG.cpp | ||
---|---|---|
237 ↗ | (On Diff #174730) | 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. |
240 ↗ | (On Diff #174730) | DeadLoopBlocks must contain it by construction, otherwise it's a bug. |
lib/Transforms/Scalar/LoopSimplifyCFG.cpp | ||
---|---|---|
240 ↗ | (On Diff #174730) | 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. |
lib/Transforms/Scalar/LoopSimplifyCFG.cpp | ||
---|---|---|
228 ↗ | (On Diff #174872) | Dont you want to add an assert here on BlocksInLoopAfterFolding + DeadLoopBlocks <= NumBlocks ? you seem to be relying on that invariant later on (when comparing this sum with numblocks for equality). |
lib/Transforms/Scalar/LoopSimplifyCFG.cpp | ||
---|---|---|
228 ↗ | (On Diff #174872) | Well, by construction BlocksInLoopAfterFolding is a subset of LiveLoopBlocks (see IsEdgeLive), and we have this: assert(L.getNumBlocks() == LiveLoopBlocks.size() + DeadLoopBlocks.size() && "Malformed block sets?"); Do you really think that what you propose is needed? |
There's an LCSSA not being preserved bug in the original landed change (@dmgreen added the test case). As part of fixing it, pls add the following options to your RUN command in the test "-verify-loop-info -verify-dom-info -verify-loop-lcssa". This will make sure all forms of verification are checked.
Underlying bug fixed, pass re-enabled and I have no failure reports for couple of days; I think we can go ahead and review this.
Me bad, it seems that some tests from https://bugs.llvm.org/show_bug.cgi?id=39783 are still failing.
Pls add -verify-memoryssa to the RUN command as well, now that MSSA is getting updated.
Comment inline.
lib/Transforms/Scalar/LoopSimplifyCFG.cpp | ||
---|---|---|
441 ↗ | (On Diff #176080) | 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). ... 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)); } |
240 ↗ | (On Diff #174730) | yes, I see that. thanks for the clarification. |
There is already a mode with memorySSA being constructed and verified in constant-fold-branch.ll:
; RUN: opt -S -enable-loop-simplifycfg-term-folding=true -loop-simplifycfg -enable-mssa-loop-dependency=true -verify-memoryssa -debug-only=loop-simplifycfg -verify-loop-info -verify-dom-info -verify-loop-lcssa 2>&1 < %s | FileCheck %s
Added asserts that we do not delete current loop and its header while dealing with dead blocks.
Patch has been reverted, likely because it triggered a bug fixed by D55357 (underlying bug, not directly connected to this patch). Waiting for confirmation, if so, I will recommit it after the fix is merged.