From 9ef950bd1ae178d73b26698a34de6d1e6aef4579 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Mon, 24 Jun 2024 13:23:36 -0700 Subject: [PATCH] Allow GC on the first instruction of NoGC region (except in prologs/epilogs) (#103540) * x64 * other platforms * no GC in epilogs * missing change for arm32 * add asserts * Addressing PR feedback. * whitespace formatting --- src/coreclr/jit/codegenarm.cpp | 10 ++--- src/coreclr/jit/codegenarm64.cpp | 13 +++--- src/coreclr/jit/codegenloongarch64.cpp | 13 +++--- src/coreclr/jit/codegenriscv64.cpp | 13 +++--- src/coreclr/jit/codegenxarch.cpp | 8 ++-- src/coreclr/jit/emit.cpp | 34 +++++++++++++++- src/coreclr/jit/emitinl.h | 8 +++- src/coreclr/jit/gcencode.cpp | 55 +++++++++++++++----------- 8 files changed, 102 insertions(+), 52 deletions(-) diff --git a/src/coreclr/jit/codegenarm.cpp b/src/coreclr/jit/codegenarm.cpp index 82deccb99ede2..fd03222c395c0 100644 --- a/src/coreclr/jit/codegenarm.cpp +++ b/src/coreclr/jit/codegenarm.cpp @@ -119,12 +119,12 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { assert(block->KindIs(BBJ_CALLFINALLY)); - GetEmitter()->emitIns_J(INS_bl, block->GetTarget()); - BasicBlock* nextBlock = block->Next(); if (block->HasFlag(BBF_RETLESS_CALL)) { + GetEmitter()->emitIns_J(INS_bl, block->GetTarget()); + if ((nextBlock == nullptr) || !BasicBlock::sameEHRegion(block, nextBlock)) { instGen(INS_BREAKPOINT); @@ -138,12 +138,12 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) // Because of the way the flowgraph is connected, the liveness info for this one instruction // after the call is not (can not be) correct in cases where a variable has a last use in the - // handler. So turn off GC reporting for this single instruction. + // handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop GetEmitter()->emitDisableGC(); - - BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); + GetEmitter()->emitIns_J(INS_bl, block->GetTarget()); // Now go to where the finally funclet needs to return to. + BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); if (nextBlock->NextIs(finallyContinuation) && !compiler->fgInDifferentRegions(nextBlock, finallyContinuation)) { // Fall-through. diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index 4ecbcc80a1ce7..da22a6d3dac14 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -2146,6 +2146,8 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { assert(block->KindIs(BBJ_CALLFINALLY)); + BasicBlock* const nextBlock = block->Next(); + // Generate a call to the finally, like this: // mov x0,qword ptr [fp + 10H] / sp // Load x0 with PSPSym, or sp if PSPSym is not used // bl finally-funclet @@ -2160,12 +2162,11 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { GetEmitter()->emitIns_Mov(INS_mov, EA_PTRSIZE, REG_R0, REG_SPBASE, /* canSkip */ false); } - GetEmitter()->emitIns_J(INS_bl_local, block->GetTarget()); - - BasicBlock* const nextBlock = block->Next(); if (block->HasFlag(BBF_RETLESS_CALL)) { + GetEmitter()->emitIns_J(INS_bl_local, block->GetTarget()); + // We have a retless call, and the last instruction generated was a call. // If the next block is in a different EH region (or is the end of the code // block), then we need to generate a breakpoint here (since it will never @@ -2182,12 +2183,12 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { // Because of the way the flowgraph is connected, the liveness info for this one instruction // after the call is not (can not be) correct in cases where a variable has a last use in the - // handler. So turn off GC reporting for this single instruction. + // handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop GetEmitter()->emitDisableGC(); - - BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); + GetEmitter()->emitIns_J(INS_bl_local, block->GetTarget()); // Now go to where the finally funclet needs to return to. + BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); if (nextBlock->NextIs(finallyContinuation) && !compiler->fgInDifferentRegions(nextBlock, finallyContinuation)) { // Fall-through. diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index 4ecdc5b2c8414..1ffbff4e82936 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -1368,6 +1368,8 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { assert(block->KindIs(BBJ_CALLFINALLY)); + BasicBlock* const nextBlock = block->Next(); + // Generate a call to the finally, like this: // mov a0,qword ptr [fp + 10H] / sp // Load a0 with PSPSym, or sp if PSPSym is not used // bl finally-funclet @@ -1382,12 +1384,11 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { GetEmitter()->emitIns_R_R_I(INS_ori, EA_PTRSIZE, REG_A0, REG_SPBASE, 0); } - GetEmitter()->emitIns_J(INS_bl, block->GetTarget()); - - BasicBlock* const nextBlock = block->Next(); if (block->HasFlag(BBF_RETLESS_CALL)) { + GetEmitter()->emitIns_J(INS_bl, block->GetTarget()); + // We have a retless call, and the last instruction generated was a call. // If the next block is in a different EH region (or is the end of the code // block), then we need to generate a breakpoint here (since it will never @@ -1404,12 +1405,12 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { // Because of the way the flowgraph is connected, the liveness info for this one instruction // after the call is not (can not be) correct in cases where a variable has a last use in the - // handler. So turn off GC reporting for this single instruction. + // handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop GetEmitter()->emitDisableGC(); - - BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); + GetEmitter()->emitIns_J(INS_bl, block->GetTarget()); // Now go to where the finally funclet needs to return to. + BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); if (nextBlock->NextIs(finallyContinuation) && !compiler->fgInDifferentRegions(nextBlock, finallyContinuation)) { // Fall-through. diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 16a4f91949c20..43799a7c7d2f3 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -1397,6 +1397,8 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { assert(block->KindIs(BBJ_CALLFINALLY)); + BasicBlock* const nextBlock = block->Next(); + // Generate a call to the finally, like this: // mov a0,qword ptr [fp + 10H] / sp // Load a0 with PSPSym, or sp if PSPSym is not used // jal finally-funclet @@ -1411,12 +1413,11 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { GetEmitter()->emitIns_R_R_I(INS_ori, EA_PTRSIZE, REG_A0, REG_SPBASE, 0); } - GetEmitter()->emitIns_J(INS_jal, block->GetTarget()); - - BasicBlock* const nextBlock = block->Next(); if (block->HasFlag(BBF_RETLESS_CALL)) { + GetEmitter()->emitIns_J(INS_jal, block->GetTarget()); + // We have a retless call, and the last instruction generated was a call. // If the next block is in a different EH region (or is the end of the code // block), then we need to generate a breakpoint here (since it will never @@ -1433,12 +1434,12 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { // Because of the way the flowgraph is connected, the liveness info for this one instruction // after the call is not (can not be) correct in cases where a variable has a last use in the - // handler. So turn off GC reporting for this single instruction. + // handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop GetEmitter()->emitDisableGC(); - - BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); + GetEmitter()->emitIns_J(INS_jal, block->GetTarget()); // Now go to where the finally funclet needs to return to. + BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); if (nextBlock->NextIs(finallyContinuation) && !compiler->fgInDifferentRegions(nextBlock, finallyContinuation)) { // Fall-through. diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index e25766249dc65..1eb90747bc80f 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -232,10 +232,11 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) { GetEmitter()->emitIns_R_S(ins_Load(TYP_I_IMPL), EA_PTRSIZE, REG_ARG_0, compiler->lvaPSPSym, 0); } - GetEmitter()->emitIns_J(INS_call, block->GetTarget()); if (block->HasFlag(BBF_RETLESS_CALL)) { + GetEmitter()->emitIns_J(INS_call, block->GetTarget()); + // We have a retless call, and the last instruction generated was a call. // If the next block is in a different EH region (or is the end of the code // block), then we need to generate a breakpoint here (since it will never @@ -253,13 +254,14 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block) #ifndef JIT32_GCENCODER // Because of the way the flowgraph is connected, the liveness info for this one instruction // after the call is not (can not be) correct in cases where a variable has a last use in the - // handler. So turn off GC reporting for this single instruction. + // handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop GetEmitter()->emitDisableGC(); #endif // JIT32_GCENCODER - BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); + GetEmitter()->emitIns_J(INS_call, block->GetTarget()); // Now go to where the finally funclet needs to return to. + BasicBlock* const finallyContinuation = nextBlock->GetFinallyContinuation(); if (nextBlock->NextIs(finallyContinuation) && !compiler->fgInDifferentRegions(nextBlock, finallyContinuation)) { diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 32d4d1e669d87..206066200a47c 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -10485,7 +10485,27 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper) } #if !defined(JIT32_GCENCODER) -// Start a new instruction group that is not interruptible +//------------------------------------------------------------------------ +// emitDisableGC: Requests that the following instruction groups are not GC-interruptible. +// +// Assumptions: +// A NoGC request can be closed by a coresponding emitEnableGC. +// Overlapping/nested NoGC requests are supported - when number of requests +// drops to zero the region is closed. This is not expected to be common. +// A completion of an epilog will close NoGC region in progress and clear the request count +// regardless of request nesting. If a block after the epilog needs to be no-GC, it needs +// to call emitDisableGC() again. +// +// Notes: +// The semantic of NoGC region is that once the first instruction executes, +// some 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. + void emitter::emitDisableGC() { assert(emitNoGCRequestCount < 10); // We really shouldn't have many nested "no gc" requests. @@ -10514,7 +10534,17 @@ void emitter::emitDisableGC() } } -// Start a new instruction group that is interruptible +//------------------------------------------------------------------------ +// emitEnableGC(): Removes a request that the following instruction groups are not GC-interruptible. +// +// Assumptions: +// We should have nonzero count of NoGC requests. +// "emitEnableGC()" reduces the number of requests by 1. +// If the number of requests goes to 0, the subsequent instruction groups are GC-interruptible. +// +// Notes: +// See emitDisableGC for more details. + void emitter::emitEnableGC() { assert(emitNoGCRequestCount > 0); diff --git a/src/coreclr/jit/emitinl.h b/src/coreclr/jit/emitinl.h index 26c48e604830b..022064073d908 100644 --- a/src/coreclr/jit/emitinl.h +++ b/src/coreclr/jit/emitinl.h @@ -589,9 +589,13 @@ bool emitter::emitGenNoGCLst(Callback& cb) { for (insGroup* ig = emitIGlist; ig; ig = ig->igNext) { - if (ig->igFlags & IGF_NOGCINTERRUPT) + if ((ig->igFlags & IGF_NOGCINTERRUPT) && ig->igSize > 0) { - if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize)) + emitter::instrDesc* id = emitFirstInstrDesc(ig->igData); + assert(id != nullptr); + assert(id->idCodeSize() > 0); + if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), + ig->igFlags & (IGF_FUNCLET_PROLOG | IGF_FUNCLET_EPILOG | IGF_EPILOG))) { return false; } diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index a8194fb26c532..7c686e64aa412 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4011,41 +4011,53 @@ void GCInfo::gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSiz // // Encoder should be either GcInfoEncoder or GcInfoEncoderWithLogging // -struct InterruptibleRangeReporter +class InterruptibleRangeReporter { - unsigned prevStart; - Encoder* gcInfoEncoderWithLog; + unsigned m_uninterruptibleEnd; + Encoder* m_gcInfoEncoder; - InterruptibleRangeReporter(unsigned _prevStart, Encoder* _gcInfo) - : prevStart(_prevStart) - , gcInfoEncoderWithLog(_gcInfo) +public: + InterruptibleRangeReporter(unsigned prologSize, Encoder* gcInfo) + : m_uninterruptibleEnd(prologSize) + , m_gcInfoEncoder(gcInfo) { } - // This callback is called for each insGroup marked with - // IGF_NOGCINTERRUPT (currently just prologs and epilogs). + // This callback is called for each insGroup marked with IGF_NOGCINTERRUPT. // Report everything between the previous region and the current // region as interruptible. - bool operator()(unsigned igFuncIdx, unsigned igOffs, unsigned igSize) + bool operator()( + unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInPrologOrEpilog) { - if (igOffs < prevStart) + if (igOffs < m_uninterruptibleEnd) { - // We're still in the main method prolog, which has already - // had it's interruptible range reported. + // We're still in the main method prolog, which we know is not interruptible. assert(igFuncIdx == 0); - assert(igOffs + igSize <= prevStart); + assert(igOffs + igSize <= m_uninterruptibleEnd); return true; } - assert(igOffs >= prevStart); - if (igOffs > prevStart) + assert(igOffs >= m_uninterruptibleEnd); + if (igOffs > m_uninterruptibleEnd) { - gcInfoEncoderWithLog->DefineInterruptibleRange(prevStart, igOffs - prevStart); + // Once the first instruction in IG executes, we cannot have GC. + // But it is ok to have GC while the IP is on the first instruction, unless we are in prolog/epilog. + unsigned interruptibleEnd = igOffs; + if (!isInPrologOrEpilog) + { + interruptibleEnd += firstInstrSize; + } + m_gcInfoEncoder->DefineInterruptibleRange(m_uninterruptibleEnd, interruptibleEnd - m_uninterruptibleEnd); } - prevStart = igOffs + igSize; + m_uninterruptibleEnd = igOffs + igSize; return true; } + + unsigned UninterruptibleEnd() + { + return m_uninterruptibleEnd; + } }; void GCInfo::gcMakeRegPtrTable( @@ -4396,17 +4408,16 @@ void GCInfo::gcMakeRegPtrTable( { assert(prologSize <= codeSize); - // Now exempt any other region marked as IGF_NOGCINTERRUPT - // Currently just prologs and epilogs. + // Now exempt any region marked as IGF_NOGCINTERRUPT InterruptibleRangeReporter reporter(prologSize, gcInfoEncoderWithLog); compiler->GetEmitter()->emitGenNoGCLst(reporter); - prologSize = reporter.prevStart; + unsigned uninterruptibleEnd = reporter.UninterruptibleEnd(); // Report any remainder - if (prologSize < codeSize) + if (uninterruptibleEnd < codeSize) { - gcInfoEncoderWithLog->DefineInterruptibleRange(prologSize, codeSize - prologSize); + gcInfoEncoderWithLog->DefineInterruptibleRange(uninterruptibleEnd, codeSize - uninterruptibleEnd); } } }