Allow GC on the first instruction of NoGC region (except in prologs/epilogs) #103540
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
VSadov
changed the title
|
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
The stress failures look preexisting. |
|
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
As compared with a baseline run #85694 with a noop change - the stress failures appear the same or fewer. |
|
@dotnet/jit-contrib |
| BasicBlock* nextBlock = block->Next(); | ||
|
|
||
| if (block->HasFlag(BBF_RETLESS_CALL)) | ||
| { | ||
| GetEmitter()->emitIns_J(INS_bl, block->GetTarget()); |
There was a problem hiding this comment.
Unlike other uses of NoGC regions, the use in genCallFinally was starting the NoGC region on instruction after the one that makes GC unsafe.
We need to agree on the same semantics for all the uses, so genCallFinally had to be adjusted to work the same as in other uses of NoGC like block copying, tailcall arg setups,...
There was a problem hiding this comment.
We could have it the other way (and adjust uses in block copying, etc..), we just need to have it the same way in all cases.
However, starting NoGC region on an instruction group that will make GC unsafe seems a lot more convenient as we may want to start NoGC region before we know exact instructions that will go into it.
| if (!isInPrologOrEpilog) | ||
| { | ||
| interruptibleEnd += firstInstrSize; | ||
| } |
There was a problem hiding this comment.
This is the key change in this PR.
| unsigned m_uninterruptibleEnd; | ||
| Encoder* m_gcInfoEncoder; |
There was a problem hiding this comment.
LGTM. Can you add some documentation on emitDisableGC (using new-style function header) that GC will be disabled only once the next instruction emitted has been executed?
Good point, Added. |
|
Thanks!! |
NoGC regions are used to temporarily disable GC in otherwise interruptible code. A typical use is during block-copying data that may contain GC references via temporaries that are not GC-reported.
The semantic of NoGC region is that once the first instruction executes, tracking invariants are violated and GC cannot happen, until the execution of the last instruction in the region makes GC safe again.
In particular - once the IP is on the first instruction, but not executed it yet, it is still safe to do GC.
The only special case is when NoGC region is used for prologs/epilogs. In such case the GC info could be incorrect until the prolog completes and epilogs may have unwindability restrictions, so the first instruction cannot have GC.
====== What prompted this change:
While working on #102680. I have discovered that sometimes we start NoGC region on the next instruction after a call. Without this change it means that once the call executes, the caller is in a NoGC state.
As a result there is a rare combination where:
That leads to an assert in
gcinfodecoder.cpp:801executionAborted.We do not expect to be asked for GC info in non-interruptible code unless it is the
executionAbortedcase.It would be nice to not mark instructions as NoGC unnecessarily. In particular we should not do that to GC-capable call sites.