JIT: move return merging earlier #93997
Conversation
Instead of merging returns to the common return block in morph, do all the merging in `fgAddInternal` (where we already did some merging). This removes a case where morph would add a control flow edge in a way that might disrupt an ongoing RPO. Earlier merging also opens up the possibility of tail merging some of the copies into the canonical return local, and possibly even some of the computations that feed the copies. Modify the flow alterations done by morph. Previously if a tail call was expressed via a call to a `CORINFO_TAILCALL_HELPER`, morph would change the block kind to `BBJ_RETURN` and then merge the return, changing the block kind to `BBJ_ALWAYS`. Since merging now happens before moprh, morph needs to leave the block kind alone. Generalize the post-tail-call sanity check in morph to recognize one new case that can come up. Contributes to dotnet#93246.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsInstead of merging returns to the common return block in morph, do all the merging in Earlier merging also opens up the possibility of tail merging some of the copies into the canonical return local, and possibly even some of the computations that feed the copies. Modify the flow alterations done by morph. Previously if a tail call was expressed via a call to a Generalize the post-tail-call sanity check in morph to recognize one new case that can come up. Contributes to #93246.
|
|
cc @dotnet/jit-contrib A modest number of diffs, small net decrease in code size. |
| // In principle we should be able to remove the proctection | ||
| // we add here, and remove the genReturnBB if it becomes unreachable, | ||
| // since we now introduce all the flow to it during this phase. |
There was a problem hiding this comment.
There's also some somewhat random code in lowering and codegen that references this block, so the deleted comment is still accurate.
There was a problem hiding this comment.
I think all those can be replaced by checks for the "right" BBJ_RETURN (the one and only without BBF_HAS_JMP), though I didn't want to push things quite that far.
|
I'm puzzled by some the CI runs... eg the libraries x64 linux failure: In my sources this assert is on line 15305. |
Because locally you don't have #93610? |
Ah, could be. Thanks. |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
libraries jitstress failure: physical promotion is adding read-backs after a tail call and this makes the post-tail call sanity checker unhappy. One of the flaws of merging returns during We could avoid this by doing the tail call conversion earlier or the return merging later. The former seems difficult. The latter loses the potential benefits we might get from tail merges of the return value copies. So neither looks great but if necessary we can defer merging until just before morph. Or we could try to avoid adding code to these spots... In this case the read backs could be avoided since along this path to the return the struct fields already have the right values... but along other paths they may not, so the writebacks in the return block would have to be hoisted into a pred for non-tail-call paths: not sure how feasible this would be. @jakobbotsch any thoughts? Even if we fix this it might be an uphill battle to keep other phases in the JIT from trying to add code after the calls. Right now it does not seem to be widespread problem. If we went this route we could beef up the diagnostic checker since we know the problematic blocks are those that end with a tail call and are marked with |
|
How many tailcalls do we lose if we just discard the cases that we cannot prove to be safe? We have |
Let me try this and see what the diff is. |
|
Going to abandon this and just teach morph RPO to tolerate new edges being added to |
ghost
locked as resolved and limited conversation to collaborators
Instead of merging returns to the common return block in morph, do all the merging in
fgAddInternal(where we already did some merging). This removes a case where morph would add a control flow edge in a way that might disrupt an ongoing RPO.Earlier merging also opens up the possibility of tail merging some of the copies into the canonical return local, and possibly even some of the computations that feed the copies.
Modify the flow alterations done by morph. Previously if a tail call was expressed via a call to a
CORINFO_TAILCALL_HELPER, morph would change the block kind toBBJ_RETURNand then merge the return, changing the block kind toBBJ_ALWAYS. Since merging now happens before moprh, morph needs to leave the block kind alone.Generalize the post-tail-call sanity check in morph to recognize one new case that can come up.
Contributes to #93246.