Skip to content

Commit

Permalink
Enable multiple nested "no GC" region requests (#68105)
Browse files Browse the repository at this point in the history
When doing fast tail call on arm32, we disable GC reporting.
But some IR nodes, like unrolled STORE_BLK, also disable GC
reporting. If one of those is within a fast tail call argument
setup region, we'll attempt to disable GC reporting twice. Since
we didn't keep track of nesting, we end up marking some of the
tail call region after the STORE_BLK as interruptible, leading
to be GC info in the argument area.

Change the enable/disable GC calls to keep a nesting count,
and only re-enable GC reporting when the count reaches zero.

Fixes #67046
  • Loading branch information
BruceForstall authored Apr 18, 2022
1 parent 2ab78dd commit 54e82d3
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 44 deletions.
86 changes: 76 additions & 10 deletions src/coreclr/jit/emit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,7 @@ insGroup* emitter::emitSavIG(bool emitAdd)
{
printf(" ; offs=%06XH, funclet=%02u, bbWeight=%s", ig->igOffs, ig->igFuncIdx,
refCntWtd2str(ig->igWeight));
emitDispIGflags(ig->igFlags);
}
else
{
Expand Down Expand Up @@ -1072,9 +1073,10 @@ void emitter::emitBegFN(bool hasFramePtr
emitJumpList = emitJumpLast = nullptr;
emitCurIGjmpList = nullptr;

emitFwdJumps = false;
emitNoGCIG = false;
emitForceNewIG = false;
emitFwdJumps = false;
emitNoGCRequestCount = 0;
emitNoGCIG = false;
emitForceNewIG = false;

#if FEATURE_LOOP_ALIGN
/* We don't have any align instructions */
Expand Down Expand Up @@ -1633,8 +1635,9 @@ void emitter::emitBegProlog()

#endif

emitNoGCIG = true;
emitForceNewIG = false;
emitNoGCRequestCount = 1;
emitNoGCIG = true;
emitForceNewIG = false;

/* Switch to the pre-allocated prolog IG */

Expand Down Expand Up @@ -1693,7 +1696,8 @@ void emitter::emitEndProlog()
{
assert(emitComp->compGeneratingProlog);

emitNoGCIG = false;
emitNoGCRequestCount = 0;
emitNoGCIG = false;

/* Save the prolog IG if non-empty or if only one block */

Expand Down Expand Up @@ -1866,7 +1870,8 @@ void emitter::emitCreatePlaceholderIG(insGroupPlaceholderType igType,
// emitter::emitDisableGC() directly. This behavior is depended upon by the fast
// tailcall implementation, which disables GC at the beginning of argument setup,
// but assumes that after the epilog it will be re-enabled.
emitNoGCIG = false;
emitNoGCRequestCount = 0;
emitNoGCIG = false;
}

emitNewIG();
Expand Down Expand Up @@ -2042,8 +2047,9 @@ void emitter::emitBegPrologEpilog(insGroup* igPh)
*/

igPh->igFlags &= ~IGF_PLACEHOLDER;
emitNoGCIG = true;
emitForceNewIG = false;
emitNoGCRequestCount = 1;
emitNoGCIG = true;
emitForceNewIG = false;

/* Set up the GC info that we stored in the placeholder */

Expand Down Expand Up @@ -2088,7 +2094,8 @@ void emitter::emitBegPrologEpilog(insGroup* igPh)

void emitter::emitEndPrologEpilog()
{
emitNoGCIG = false;
emitNoGCRequestCount = 0;
emitNoGCIG = false;

/* Save the IG if non-empty */

Expand Down Expand Up @@ -9384,3 +9391,62 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper)

return result;
}

#if !defined(JIT32_GCENCODER)
// Start a new instruction group that is not interruptable
void emitter::emitDisableGC()
{
assert(emitNoGCRequestCount < 10); // We really shouldn't have many nested "no gc" requests.
++emitNoGCRequestCount;

if (emitNoGCRequestCount == 1)
{
JITDUMP("Disable GC\n");
assert(!emitNoGCIG);

emitNoGCIG = true;

if (emitCurIGnonEmpty())
{
emitNxtIG(true);
}
else
{
emitCurIG->igFlags |= IGF_NOGCINTERRUPT;
}
}
else
{
JITDUMP("Disable GC: %u no-gc requests\n", emitNoGCRequestCount);
assert(emitNoGCIG);
}
}

// Start a new instruction group that is interruptable
void emitter::emitEnableGC()
{
assert(emitNoGCRequestCount > 0);
assert(emitNoGCIG);
--emitNoGCRequestCount;

if (emitNoGCRequestCount == 0)
{
JITDUMP("Enable GC\n");

emitNoGCIG = false;

// The next time an instruction needs to be generated, force a new instruction group.
// It will be an emitAdd group in that case. Note that the next thing we see might be
// a label, which will force a non-emitAdd group.
//
// Note that we can't just create a new instruction group here, because we don't know
// if there are going to be any instructions added to it, and we don't support empty
// instruction groups.
emitForceNewIG = true;
}
else
{
JITDUMP("Enable GC: still %u no-gc requests\n", emitNoGCRequestCount);
}
}
#endif // !defined(JIT32_GCENCODER)
37 changes: 3 additions & 34 deletions src/coreclr/jit/emit.h
Original file line number Diff line number Diff line change
Expand Up @@ -2014,8 +2014,9 @@ class emitter

void emitCheckFuncletBranch(instrDesc* jmp, insGroup* jmpIG); // Check for illegal branches between funclets

bool emitFwdJumps; // forward jumps present?
bool emitNoGCIG; // Are we generating IGF_NOGCINTERRUPT insGroups (for prologs, epilogs, etc.)
bool emitFwdJumps; // forward jumps present?
unsigned emitNoGCRequestCount; // Count of number of nested "NO GC" region requests we have.
bool emitNoGCIG; // Are we generating IGF_NOGCINTERRUPT insGroups (for prologs, epilogs, etc.)
bool emitForceNewIG; // If we generate an instruction, and not another instruction group, force create a new emitAdd
// instruction group.

Expand Down Expand Up @@ -3309,38 +3310,6 @@ inline void emitter::emitNewIG()
emitGenIG(ig);
}

#if !defined(JIT32_GCENCODER)
// Start a new instruction group that is not interruptable
inline void emitter::emitDisableGC()
{
emitNoGCIG = true;

if (emitCurIGnonEmpty())
{
emitNxtIG(true);
}
else
{
emitCurIG->igFlags |= IGF_NOGCINTERRUPT;
}
}

// Start a new instruction group that is interruptable
inline void emitter::emitEnableGC()
{
emitNoGCIG = false;

// The next time an instruction needs to be generated, force a new instruction group.
// It will be an emitAdd group in that case. Note that the next thing we see might be
// a label, which will force a non-emitAdd group.
//
// Note that we can't just create a new instruction group here, because we don't know
// if there are going to be any instructions added to it, and we don't support empty
// instruction groups.
emitForceNewIG = true;
}
#endif // !defined(JIT32_GCENCODER)

/*****************************************************************************/
#endif // _EMIT_H_
/*****************************************************************************/

0 comments on commit 54e82d3

Please sign in to comment.