JIT: Refactor Compiler::optRedirectBlock; remove BasicBlock::CopyTarget #98526
Conversation
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsPart of #93020. When cloning a set of basic blocks (such as during loop cloning, finally cloning, etc.), the successors of the copied blocks may be different from the successors of their original counterparts, due to the successors being copied as well. We currently handle this as so:
This PR simplifies this process in preparation for updating
@AndyAyersMS I finished refactoring
|
|
|
||
| case BBJ_EHCATCHRET: | ||
| case BBJ_EHFILTERRET: | ||
| // These block types should not need redirecting |
There was a problem hiding this comment.
For context, the previous optRedirectBlock implementation did nothing for these types.
Diff results for #98526Assembly diffsAssembly diffs for windows/arm64 ran on linux/x64Diffs are based on 1,454,211 contexts (555,875 MinOpts, 898,336 FullOpts). Overall (+0 bytes)
FullOpts (+0 bytes)
Details here Throughput diffsThroughput diffs for osx/arm64 ran on linux/x64Overall (-0.02% to -0.01%)
MinOpts (-0.00% to +0.01%)
FullOpts (-0.02% to -0.01%)
Throughput diffs for windows/arm64 ran on linux/x64Overall (-0.02% to -0.01%)
MinOpts (-0.00% to +0.01%)
FullOpts (-0.02% to -0.01%)
Throughput diffs for windows/x64 ran on linux/x64Overall (-0.02% to -0.01%)
FullOpts (-0.02% to -0.01%)
Details here |
Diff results for #98526Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,533,745 contexts (1,007,248 MinOpts, 1,526,497 FullOpts). MISSED contexts: 3 (0.00%) Overall (-4 bytes)
FullOpts (-4 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,573,319 contexts (1,008,940 MinOpts, 1,564,379 FullOpts). Overall (+49 bytes)
FullOpts (+49 bytes)
Details here Assembly diffs for linux/arm ran on windows/x86Diffs are based on 2,224,699 contexts (831,156 MinOpts, 1,393,543 FullOpts). MISSED contexts: 73,372 (3.19%) Overall (+2 bytes)
FullOpts (+2 bytes)
Details here Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64Overall (-0.02% to -0.01%)
MinOpts (-0.01% to +0.00%)
FullOpts (-0.02% to -0.01%)
Throughput diffs for linux/x64 ran on windows/x64Overall (-0.02% to -0.01%)
FullOpts (-0.02% to -0.01%)
Details here |
|
cc @dotnet/jit-contrib, @AndyAyersMS PTAL. The diffs are from slight changes in edge weights/likelihoods. |
| } | ||
|
|
||
| fgAddRefPred(trueTarget, newBlk); |
There was a problem hiding this comment.
Should we be passing in the "inspiring edge" here (the one from original source to original target)?
There was a problem hiding this comment.
I was planning on doing that once we have easy access to successor edges -- something like fgAddRefPred(trueTarget, newBlk, blk->GetTrueEdge()). In the meantime, I can add calls to fgGetPredForBlock in to get the inspiring edge, since the TP impact will be temporary, or I can include this in my upcoming PRs for adding successor edge members to BasicBlock -- do you have a preference?
| } | ||
|
|
||
| fgAddRefPred(*jumpPtr, newBlk); |
There was a problem hiding this comment.
Ditto here, though it can be tricker for edges that have dup counts.
| void optRedirectBlock(BasicBlock* blk, | ||
| BlockToBlockMap* redirectMap, | ||
| const RedirectBlockOption = RedirectBlockOption::DoNotChangePredLists); | ||
| BasicBlock* newBlk, | ||
| BlockToBlockMap* redirectMap); |
There was a problem hiding this comment.
I'm not sure I understand the new function that well -- it requires me to have a block around that the previous function did not require me to have around, but what if I don't have that?
This function doesn't seem to be doing redirection anymore, it rather seems to be initializing a new block based on an old block and a map. Which is fine if that's what we generally need, but the old function seemed much more general than this -- basically a generalization of fgReplaceJumpTarget to replace multiple targets at once. How would we implement that functionality with the new scheme? It is conceivable to me that we are going to need it again.
There was a problem hiding this comment.
it requires me to have a block around that the previous function did not require me to have around, but what if I don't have that?
For the current callers of optRedirectBlock, newBlk is obtained from redirectMap -- for each copied block blk, we expect there to be a blk -> newBlk mapping. I could refactor optRedirectBlock slightly so that it only takes blk, and retrieves newBlk from redirectMap. We'd always expect this mapping to exist, right? Can you foresee a situation where we wouldn't have that?
This function doesn't seem to be doing redirection anymore, it rather seems to be initializing a new block based on an old block and a map. Which is fine if that's what we generally need, but the old function seemed much more general than this
In my opinion, the old version was trying to do a few too many things, making the semantics of its callers a bit awkward. Previously, callers would have to copy the old block's target(s) to the new block with BasicBlock::CopyTarget without setting up any pred edges, and then tell optRedirectBlock to add pred edges, while deciding what the real target should be. I don't think this approach will hold up well once we've replaced block targets with successor edges: Once we've done that, it won't be possible for a block to have a jump target without an edge to it, which is a state we'd previously establish before calling optRedirectBlock.
How would we implement that functionality with the new scheme? It is conceivable to me that we are going to need it again.
This would introduce some code duplication, but maybe we could have a version of optRedirectBlock that assumes newBlk's successors are initialized, and the current version (which we can rename to imply it is initializing successors, if you'd prefer) can continue to assume successors aren't initialized?
My next few PRs will be touching this method, so I'm happy to add any follow-up changes you recommend to those.
There was a problem hiding this comment.
For the current callers of optRedirectBlock, newBlk is obtained from redirectMap -- for each copied block blk, we expect there to be a blk -> newBlk mapping. I could refactor optRedirectBlock slightly so that it only takes blk, and retrieves newBlk from redirectMap. We'd always expect this mapping to exist, right? Can you foresee a situation where we wouldn't have that?
I would not expect this mapping to always exist. As I mentioned above, I think optRedirectBlock should be viewed as a generalization of fgReplaceJumpTarget that allows replacing multiple targets in one operation. I.e. it was an operation similar to
for ((BasicBlock* key, BasicBlock* target) in blockMap) { fgReplaceJumpTarget(block, key, target); }I agree the overloaded handling around preds before wasn't very pretty.
I think with the refactoring done in this PR the function should be renamed to something that indicates that it operates on a partially initialized block and is some form of initialization -- it no longer matches the above semantics. Maybe something like optInitializeDuplicatedBlockTargets?
I actually had local changes where I generalized optRedirectBlock to take a functor instead of a BlockToBlockMap, making it more easy to use for these kinds of more general block redirections. With that I think fgReplaceJumpTarget could be implemented in terms of optRedirectBlock (with a functor like [=](BasicBlock* target) { if (target == oldBlock) { return newBlock; } return target; }).
There was a problem hiding this comment.
I think with the refactoring done in this PR the function should be renamed to something that indicates that it operates on a partially initialized block and is some form of initialization -- it no longer matches the above semantics. Maybe something like optInitializeDuplicatedBlockTargets?
Sure thing, I can include that change in my next PR.
I actually had local changes where I generalized optRedirectBlock to take a functor instead of a BlockToBlockMap, making it more easy to use for these kinds of more general block redirections.
I like that idea. Are you ok with me wrapping up the successor edge refactor first, and then coming back to this? That should only take a few more PRs; I have the code ready locally.
There was a problem hiding this comment.
Sure -- also, I'm ok with leaving the function out entirely and leaving it as it is right now (with the rename). We can add the more general version when we find a need for it.
|
For my next few PRs, I plan to incrementally convert each block kind to use successor edges instead of block targets. With each update to |
Part of #93020. When cloning a set of basic blocks (such as during loop cloning, finally cloning, etc.), the successors of the copied blocks may be different from the successors of their original counterparts, due to the successors being copied as well. We currently handle this as so:
BasicBlock::CopyTarget; don't create any edges yet, as the new block's target(s) may change.Compiler::optRedirectBlock, determine if the new block's target needs to be redirected to the copy of the original target, using a mapping of original blocks to their copies. We may or may not also initialize/update the successor edges for the new block.This PR simplifies this process in preparation for updating
BasicBlockto track its successors viaFlowEdgepointers, instead ofBasicBlockpointers. This is the new process:Compiler::optRedirectBlock, for each successor of the original block, determine if the new block should use the successor, or if the new block should be redirected to a copy of that successor, using the block map. Either way, all of the new block's successor edges must be initialized here.@AndyAyersMS I finished refactoring
BasicBlockto use flow edges to access its successors locally. That change is quite large, so I'm trying to move backwards and open PRs for the more modular bits of the refactor, starting withoptRedirectBlock.