Clone blocks that are reachable from outside the loop. #19570
Conversation
|
@swift-ci please test tensorflow |
bgogul
force-pushed
the
sese_clone_blocks
branch
from
45376e1 to
674f28f
Compare
| SILBasicBlock *clonedSucc = cloner.cloneBlock(succ); | ||
| changeBranchTarget(current->getTerminator(), edgeIdx, clonedSucc, | ||
| /*preserveArgs*/ true); | ||
| worklist.insert(clonedSucc); | ||
| continue; | ||
| } | ||
| worklist.insert(succ); |
There was a problem hiding this comment.
the code might be easier to read due to reduced indentation, if we negate "if (!DI->properlyDominates(header, succ))".
| // header directly to the demuxBlock created in the loop below. Alternatively, | ||
| // we can also use contractUncondBranches in TFParititon.cpp to remove this | ||
| // block later. | ||
| // Create a new exit block. Strictly, we don't need this block, but it makes |
There was a problem hiding this comment.
"we don't need this block" or "we don't always need this block"? If the former, what does "if appropriate" 2 lines below mean?
There was a problem hiding this comment.
It should "don't always need the block". Fixed the comment.
| // TODO(https://bugs.swift.org/browse/SR-8336): the transformations here are | ||
| // simple that we should be able to incrementally update the DI & PDI. | ||
| DI->recalculate(*currentFn); | ||
| PDI->recalculate(*currentFn); |
There was a problem hiding this comment.
FWIW, I think this approach is totally fine, assuming that this isn't commonly necessary. If it is commonly necessary, it would be interesting to see what the patterns are and maybe a trivial local update would work.
There was a problem hiding this comment.
Thanks! This should not be commonly necessary as it only happens when we clone blocks. I will leave it for now.
bgogul
force-pushed
the
sese_clone_blocks
branch
from
674f28f to
01c9abd
Compare
|
Looks good! |
bgogul
force-pushed
the
sese_clone_blocks
branch
from
01c9abd to
8eb9f23
Compare
|
@swift-ci please test tensorflow |
When performing SESE canonicalization, it is necessary to clone some blocks. Consider the following CFG (6 -> 4 is the backedge):
This is an example where the nodes that lead to the common post dominator of all exit nodes is also reachable from nodes outside of the loop. The key issue is that bb9 is the common post dominator of the exit blocks bb5 and bb8 of the loop. As a first step, we attempt to make the common post dominator the single exit block. In order to do that, we need to move the blocks reachable from exit blocks bb5 and bb8 up to bb9 into the loop. In this example, bb3 needs to be moved into the loop, but is reachable from bb1 (outside the loop) and bb8 (which will be moved inside the loop). To deal with this case, we simply clone bb3 before moving.
This PR also has a bugfix in the dominator patching logic in mergeBasicBlockWithSuccessor.