[Mem2Reg] Handled unreachables for single-block allocations. #39930
Conversation
|
@swift-ci please test |
nate-chandler
force-pushed
the
lexical_lifetimes/mem2reg-end-single-block-lifetimes-with-valid-memory
branch
from
6459877 to
eb8eb5e
Compare
|
@swift-ci please test |
|
@swift-ci please clean test windows platform |
Uses of an alloc_stack instruction in an unreachable block must also be unreachable. Don't waste time optimizing unreachable code.
Previously, it was asserted that any single-block allocation which had valid memory after all instructions in the block were visited terminated in unreachable. That assertion was false--while all paths through that block must end in unreachable, the block itself need not be terminated with an unreachable inst. Here, that is corrected by walking forward from the block until blocks terminated by unreachables or blocks not dominated by the block containing the alloc_stack are reached. In the first case, the lifetime is ended just before the unreachable inst. In the second, the lifetime is ended just before the branch to such a successor block (i.e. just before the branch to a block which is not dominated by the block containing the alloc_stack).
nate-chandler
force-pushed
the
lexical_lifetimes/mem2reg-end-single-block-lifetimes-with-valid-memory
branch
from
eb8eb5e to
797eab1
Compare
|
@swift-ci please test |
| runningVals->value); | ||
| // There is still valid storage after visiting all instructions in this | ||
| // block which are the only instructions involving this alloc_stack. | ||
| // This can only happen if all paths from this block end in unreachable. |
There was a problem hiding this comment.
Can you give a SIL example here.
There was a problem hiding this comment.
I talked with Nate. I am ok with this actually.
nate-chandler
deleted the
lexical_lifetimes/mem2reg-end-single-block-lifetimes-with-valid-memory
branch
There was a problem hiding this comment.
@nate-chandler this fix looks great. I made a couple of low-priority requests.
| continue; | ||
| } | ||
| bool endedLifetime = false; | ||
| for (auto *successor : block->getSuccessorBlocks()) { |
There was a problem hiding this comment.
This loop confuses me. A block with multiple successors must dominate all of them. You should be able to call something like getSingleSuccessor and only do the dominance check in that case, without any loop or confusing control flow based on a boolean flag.
To sanity check this, you can just assert that parentBlock dominates each block popped from the worklist immediately after popping it.
| unreachable | ||
| } | ||
|
|
||
| // CHECK-LABEL: sil [ossa] @unreachable_6 : $@convention(thin) (@owned C) -> () { |
There was a problem hiding this comment.
None of these unit test are commented. Outside the context of this PR it wouldn't be obvious what's being tested. A single line like: "bailout on alloc_stack in unreachable block" helps. When updating dozens of SIL tests cases, it's not practical to look at the git log for all of them.
I know no one else comments tests, but I still think it's important.
Previously, it was asserted that any single-block allocation which had valid memory after all instructions in the block were visited terminated in unreachable. That assertion was false--while all paths through that block must end in unreachable, the block itself need not be terminated with an unreachable inst. Here, that is corrected by walking forward from the block until blocks terminated by unreachables or blocks to which there are paths that do not go through the block containing the alloc_stack are reached. In the first case, the lifetime is ended just before the unreachable inst. In the second, the lifetime is ended just before the branch to such a successor block.