Skip Upper vector save/restore for blocks that THROW #66109
Conversation
…k is a THROW block
…net#66062)" This reverts commit eb2fea8.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsCurrently, we save and restore vectors for This also reverts #66062. Fixes: #65332
|
|
/azp run runtime-coreclr superpmi-replay, runtime-coreclr jitstressregs |
|
@dotnet/jit-contrib , @BruceForstall |
|
Azure Pipelines successfully started running 2 pipeline(s). |
src/coreclr/jit/lsrabuild.cpp
Outdated
| { | ||
| if ((tree != nullptr) && tree->IsCall()) | ||
| { | ||
| if (tree->AsCall()->IsNoReturn()) |
There was a problem hiding this comment.
You might also want to check fgIsThrow here but I assume both IsNoreReturn and fgIsThrow should always be inside a BBJ_THROW block
There was a problem hiding this comment.
I looked around and seems that the only non-throw case where calls are marked as NoReturn is here:
runtime/src/coreclr/jit/fgbasic.cpp
Line 2138 in 24ce527
I am not familiar with how that translates to during LSRA.
src/coreclr/jit/lsrabuild.cpp
Outdated
| } | ||
| } | ||
|
|
||
| if ((compiler->compCurBB != nullptr) && (compiler->compCurBB->KindIs(BBJ_THROW))) |
There was a problem hiding this comment.
Perhaps, block should a parameter, or method renamed to something stating it checks current block?
There was a problem hiding this comment.
I thought about passing block as a parameter but there are lot of call chain where I would have to pass block. We use compCurBB in earlier phases as well as curBBNum in LSRA to avoid doing that. As far method name, I think I have stated in the comments that it inspects the block.
src/coreclr/jit/lsrabuild.cpp
Outdated
| { | ||
| if (tree->AsCall()->IsNoReturn()) | ||
| { | ||
| // No point in having vector save/restore if the call will not return. |
There was a problem hiding this comment.
It's not clear to me why this would be safe. Couldn't you have computation after a call that requires restoring the registers, and before a throw?
There was a problem hiding this comment.
Seems like it's only safe if there are not uses of the (restored) register between this point and the throw, in which case the restore is "dead code".
There was a problem hiding this comment.
Vector restore happens at two places:
- At the end of every block, whenever any
largeVectorVarswas partially spilled - At the locations where we see the use
While it should be safe to eliminate the restores (and hence the saves) that happen for #1 above if the block is a THROW block, but there can be a scenario (as you pointed out) where eliminating save/restore will be wrong:
save(Vx) ; <- shouldn't be eliminated because of the use
CALL ...
Vx = restore() ; <- shouldn't be eliminated because of the use
= use(Vx)
...
THROWSince UpperVectorSave refpositions are created when we build kills, it is hard to look ahead to see which ones of them will be used (and hence restored) and thus can be eliminated. For e.g. in following case, we can (should) eliminate the unnecessary save/restore.
save(Vx) ; <- safe to eliminate
CALL ...
Vx = restore() ; <- safe to eliminate
...
THROWTo summarize, I was trying to eliminate vector saves, but it might not be safe (as Bruce pointed), so I will just try to at least eliminate the restore that happens at the end of the THROW block.
There was a problem hiding this comment.
Hhm, looks like our model is not built such that it can withstand to see only saves and not (optimally removed) restores.
There was a problem hiding this comment.
Pushed an attempt to track for a block that throws, if a spill done before the call will ever be used. If not, skip save/restore for them. While I am at it, I will check what it takes to fix the problem @sandreenko pointed out in #59315.
ghost
added
needs-author-action
|
/azp run runtime-coreclr superpmi-replay, runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The failures seems to be infrastructure issue. superpmi-replay is green which used to fail earlier. |
|
Failure is #65455. |
3 participants
Currently, we save and restore vectors for
BBJ_THROWblocks as well, which is not really needed. In #62662, we avoiding doing save/restore for calls that do not return. This PR extends the logic to also skip save/restore if the block ends with throw.This also reverts #66062.
Fixes: #65332