Skip to content

JIT: Switch arm32 call-finally to be similar to other targets #95117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 26, 2023
Merged

JIT: Switch arm32 call-finally to be similar to other targets #95117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4a29e4
JIT: Switch arm32 call-finally to be similar to other targets
jakobbotsch Nov 22, 2023
5690e19
Set FEATURE_EH_CALLFINALLY_THUNKS = 1 for arm32
jakobbotsch Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,12 +510,6 @@ void BasicBlock::dspFlags()
{
printf("nullcheck ");
}
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
if (bbFlags & BBF_FINALLY_TARGET)
{
printf("ftarget ");
}
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
if (bbFlags & BBF_BACKWARD_JUMP)
{
printf("bwd ");
Expand Down Expand Up @@ -1661,16 +1655,8 @@ BasicBlock* BasicBlock::bbNewBasicBlock(Compiler* compiler, BBjumpKinds jumpKind
//
bool BasicBlock::isBBCallAlwaysPair() const
{
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
if (this->KindIs(BBJ_CALLFINALLY))
#else
if (this->KindIs(BBJ_CALLFINALLY) && !(this->bbFlags & BBF_RETLESS_CALL))
#endif
{
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
// On ARM, there are no retless BBJ_CALLFINALLY.
assert(!(this->bbFlags & BBF_RETLESS_CALL));
#endif
// Some asserts that the next block is a BBJ_ALWAYS of the proper form.
assert(!this->IsLast());
assert(this->Next()->KindIs(BBJ_ALWAYS));
Expand Down
49 changes: 17 additions & 32 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,38 +409,27 @@ enum BasicBlockFlags : unsigned __int64
BBF_HAS_MDARRAYREF = MAKE_BBFLAG(24), // Block has a multi-dimensional array reference
BBF_HAS_NEWOBJ = MAKE_BBFLAG(25), // BB contains 'new' of an object type.

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

BBF_FINALLY_TARGET = MAKE_BBFLAG(26), // BB is the target of a finally return: where a finally will return during
// non-exceptional flow. Because the ARM calling sequence for calling a
// finally explicitly sets the return address to the finally target and jumps
// to the finally, instead of using a call instruction, ARM needs this to
// generate correct code at the finally target, to allow for proper stack
// unwind from within a non-exceptional call to a finally.

#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

BBF_RETLESS_CALL = MAKE_BBFLAG(27), // BBJ_CALLFINALLY that will never return (and therefore, won't need a paired
BBF_RETLESS_CALL = MAKE_BBFLAG(26), // BBJ_CALLFINALLY that will never return (and therefore, won't need a paired
// BBJ_ALWAYS); see isBBCallAlwaysPair().
BBF_LOOP_PREHEADER = MAKE_BBFLAG(28), // BB is a loop preheader block
BBF_COLD = MAKE_BBFLAG(29), // BB is cold
BBF_PROF_WEIGHT = MAKE_BBFLAG(30), // BB weight is computed from profile data
BBF_KEEP_BBJ_ALWAYS = MAKE_BBFLAG(31), // A special BBJ_ALWAYS block, used by EH code generation. Keep the jump kind
BBF_LOOP_PREHEADER = MAKE_BBFLAG(27), // BB is a loop preheader block
BBF_COLD = MAKE_BBFLAG(28), // BB is cold
BBF_PROF_WEIGHT = MAKE_BBFLAG(29), // BB weight is computed from profile data
BBF_KEEP_BBJ_ALWAYS = MAKE_BBFLAG(30), // A special BBJ_ALWAYS block, used by EH code generation. Keep the jump kind
// as BBJ_ALWAYS. Used for the paired BBJ_ALWAYS block following the
// BBJ_CALLFINALLY block, as well as, on x86, the final step block out of a
// finally.
BBF_HAS_CALL = MAKE_BBFLAG(32), // BB contains a call
BBF_DOMINATED_BY_EXCEPTIONAL_ENTRY = MAKE_BBFLAG(33), // Block is dominated by exceptional entry.
BBF_BACKWARD_JUMP = MAKE_BBFLAG(34), // BB is surrounded by a backward jump/switch arc
BBF_BACKWARD_JUMP_SOURCE = MAKE_BBFLAG(35), // Block is a source of a backward jump
BBF_BACKWARD_JUMP_TARGET = MAKE_BBFLAG(36), // Block is a target of a backward jump
BBF_PATCHPOINT = MAKE_BBFLAG(37), // Block is a patchpoint
BBF_PARTIAL_COMPILATION_PATCHPOINT = MAKE_BBFLAG(38), // Block is a partial compilation patchpoint
BBF_HAS_HISTOGRAM_PROFILE = MAKE_BBFLAG(39), // BB contains a call needing a histogram profile
BBF_TAILCALL_SUCCESSOR = MAKE_BBFLAG(40), // BB has pred that has potential tail call
BBF_RECURSIVE_TAILCALL = MAKE_BBFLAG(41), // Block has recursive tailcall that may turn into a loop
BBF_NO_CSE_IN = MAKE_BBFLAG(42), // Block should kill off any incoming CSE
BBF_CAN_ADD_PRED = MAKE_BBFLAG(43), // Ok to add pred edge to this block, even when "safe" edge creation disabled
BBF_HAS_CALL = MAKE_BBFLAG(31), // BB contains a call
BBF_DOMINATED_BY_EXCEPTIONAL_ENTRY = MAKE_BBFLAG(32), // Block is dominated by exceptional entry.
BBF_BACKWARD_JUMP = MAKE_BBFLAG(33), // BB is surrounded by a backward jump/switch arc
BBF_BACKWARD_JUMP_SOURCE = MAKE_BBFLAG(34), // Block is a source of a backward jump
BBF_BACKWARD_JUMP_TARGET = MAKE_BBFLAG(35), // Block is a target of a backward jump
BBF_PATCHPOINT = MAKE_BBFLAG(36), // Block is a patchpoint
BBF_PARTIAL_COMPILATION_PATCHPOINT = MAKE_BBFLAG(37), // Block is a partial compilation patchpoint
BBF_HAS_HISTOGRAM_PROFILE = MAKE_BBFLAG(38), // BB contains a call needing a histogram profile
BBF_TAILCALL_SUCCESSOR = MAKE_BBFLAG(39), // BB has pred that has potential tail call
BBF_RECURSIVE_TAILCALL = MAKE_BBFLAG(40), // Block has recursive tailcall that may turn into a loop
BBF_NO_CSE_IN = MAKE_BBFLAG(41), // Block should kill off any incoming CSE
BBF_CAN_ADD_PRED = MAKE_BBFLAG(42), // Ok to add pred edge to this block, even when "safe" edge creation disabled

// The following are sets of flags.

Expand Down Expand Up @@ -1248,10 +1237,6 @@ struct BasicBlock : private LIR::Range

void* bbEmitCookie;

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
void* bbUnwindNopEmitCookie;
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

#ifdef VERIFIER
stackDesc bbStackIn; // stack descriptor for input
stackDesc bbStackOut; // stack descriptor for output
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/jit/codegen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,9 +626,6 @@ class CodeGen final : public CodeGenInterface
void genSetPSPSym(regNumber initReg, bool* pInitRegZeroed);

void genUpdateCurrentFunclet(BasicBlock* block);
#if defined(TARGET_ARM)
void genInsertNopForUnwinder(BasicBlock* block);
#endif

#else // !FEATURE_EH_FUNCLETS

Expand Down
84 changes: 39 additions & 45 deletions src/coreclr/jit/codegenarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,33 +117,55 @@ bool CodeGen::genStackPointerAdjustment(ssize_t spDelta, regNumber tmpReg)
//
BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
{
BasicBlock* bbFinallyRet = nullptr;
GetEmitter()->emitIns_J(INS_bl, block->GetJumpDest());

// We don't have retless calls, since we use the BBJ_ALWAYS to point at a NOP pad where
// we would have otherwise created retless calls.
assert(block->isBBCallAlwaysPair());
BasicBlock* nextBlock = block->Next();

assert(!block->IsLast());
assert(block->Next()->KindIs(BBJ_ALWAYS));
assert(block->Next()->HasJump());
assert(block->Next()->GetJumpDest()->bbFlags & BBF_FINALLY_TARGET);
if (block->bbFlags & BBF_RETLESS_CALL)
{
if ((nextBlock == nullptr) || !BasicBlock::sameEHRegion(block, nextBlock))
{
instGen(INS_BREAKPOINT);
}
}
else
{
assert((nextBlock != nullptr) && nextBlock->isBBCallAlwaysPairTail());

// 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.
GetEmitter()->emitDisableGC();

bbFinallyRet = block->Next()->GetJumpDest();
BasicBlock* const jumpDest = nextBlock->GetJumpDest();

// Load the address where the finally funclet should return into LR.
// The funclet prolog/epilog will do "push {lr}" / "pop {pc}" to do the return.
genMov32RelocatableDisplacement(bbFinallyRet, REG_LR);
// Now go to where the finally funclet needs to return to.
if (nextBlock->NextIs(jumpDest) && !compiler->fgInDifferentRegions(nextBlock, jumpDest))
{
// Fall-through.
// TODO-ARM-CQ: Can we get rid of this instruction, and just have the call return directly
// to the next instruction? This would depend on stack walking from within the finally
// handler working without this instruction being in this special EH region.
instGen(INS_nop);
}
else
{
GetEmitter()->emitIns_J(INS_b, jumpDest);
}

// Jump to the finally BB
inst_JMP(EJ_jmp, block->GetJumpDest());
GetEmitter()->emitEnableGC();
}

// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
assert(!(block->bbFlags & BBF_RETLESS_CALL));
assert(block->isBBCallAlwaysPair());
return block->Next();
if (!(block->bbFlags & BBF_RETLESS_CALL))
{
assert(block->isBBCallAlwaysPair());
block = nextBlock;
}
return block;
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -2572,34 +2594,6 @@ void CodeGen::genSetPSPSym(regNumber initReg, bool* pInitRegZeroed)
GetEmitter()->emitIns_S_R(INS_str, EA_PTRSIZE, regTmp, compiler->lvaPSPSym, 0);
}

void CodeGen::genInsertNopForUnwinder(BasicBlock* block)
{
// If this block is the target of a finally return, we need to add a preceding NOP, in the same EH region,
// so the unwinder doesn't get confused by our "movw lr, xxx; movt lr, xxx; b Lyyy" calling convention that
// calls the funclet during non-exceptional control flow.
if (block->bbFlags & BBF_FINALLY_TARGET)
{
assert(block->bbFlags & BBF_HAS_LABEL);

#ifdef DEBUG
if (compiler->verbose)
{
printf("\nEmitting finally target NOP predecessor for " FMT_BB "\n", block->bbNum);
}
#endif
// Create a label that we'll use for computing the start of an EH region, if this block is
// at the beginning of such a region. If we used the existing bbEmitCookie as is for
// determining the EH regions, then this NOP would end up outside of the region, if this
// block starts an EH region. If we pointed the existing bbEmitCookie here, then the NOP
// would be executed, which we would prefer not to do.

block->bbUnwindNopEmitCookie = GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur,
gcInfo.gcRegByrefSetCur, false DEBUG_ARG(block));

instGen(INS_nop);
}
}

//-----------------------------------------------------------------------------
// genZeroInitFrameUsingBlockInit: architecture-specific helper for genZeroInitFrame in the case
// `genUseBlockInit` is set.
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/codegencommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,7 +909,7 @@ void CodeGen::genDefineTempLabel(BasicBlock* label)
{
genLogLabel(label);
label->bbEmitCookie = GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur,
gcInfo.gcRegByrefSetCur, false DEBUG_ARG(label));
gcInfo.gcRegByrefSetCur DEBUG_ARG(label));
}

// genDefineInlineTempLabel: Define an inline label that does not affect the GC
Expand Down
6 changes: 1 addition & 5 deletions src/coreclr/jit/codegenlinear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,10 +290,6 @@ void CodeGen::genCodeForBBlist()
}
}

#if defined(TARGET_ARM)
genInsertNopForUnwinder(block);
#endif

/* Start a new code output block */

genUpdateCurrentFunclet(block);
Expand Down Expand Up @@ -351,7 +347,7 @@ void CodeGen::genCodeForBBlist()
// Mark a label and update the current set of live GC refs

block->bbEmitCookie = GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur,
gcInfo.gcRegByrefSetCur, false DEBUG_ARG(block));
gcInfo.gcRegByrefSetCur DEBUG_ARG(block));
}

if (block->IsFirstColdBlock(compiler))
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4613,14 +4613,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
DoPhase(this, PHASE_CLONE_FINALLY, &Compiler::fgCloneFinally);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

// Update finally target flags after EH optimizations
//
DoPhase(this, PHASE_UPDATE_FINALLY_FLAGS, &Compiler::fgUpdateFinallyTargetFlags);

#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

#if DEBUG
if (lvaEnregEHVars)
{
Expand Down
21 changes: 0 additions & 21 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4645,13 +4645,6 @@ class Compiler

BlockSet fgEnterBlks; // Set of blocks which have a special transfer of control; the "entry" blocks plus EH handler
// begin blocks.
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
BlockSet fgAlwaysBlks; // Set of blocks which are BBJ_ALWAYS part of BBJ_CALLFINALLY/BBJ_ALWAYS pair that should
// never be removed due to a requirement to use the BBJ_ALWAYS for generating code and
// not have "retless" blocks.

#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

#ifdef DEBUG
bool fgReachabilitySetsValid; // Are the bbReach sets valid?
bool fgEnterBlksSetValid; // Is the fgEnterBlks set valid?
Expand Down Expand Up @@ -4750,17 +4743,6 @@ class Compiler

void fgCleanupContinuation(BasicBlock* continuation);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

PhaseStatus fgUpdateFinallyTargetFlags();

void fgClearAllFinallyTargetBits();

void fgAddFinallyTargetFlags();

void fgFixFinallyTargetFlags(BasicBlock* pred, BasicBlock* succ, BasicBlock* newBlock);

#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
PhaseStatus fgTailMergeThrows();
void fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock,
BasicBlock* nonCanonicalBlock,
Expand Down Expand Up @@ -5557,9 +5539,6 @@ class Compiler
BasicBlock* fgRelocateEHRange(unsigned regionIndex, FG_RELOCATE_TYPE relocateType);

#if defined(FEATURE_EH_FUNCLETS)
#if defined(TARGET_ARM)
void fgClearFinallyTargetBit(BasicBlock* block);
#endif // defined(TARGET_ARM)
bool fgIsIntraHandlerPred(BasicBlock* predBlock, BasicBlock* block);
bool fgAnyIntraHandlerPreds(BasicBlock* block);
void fgInsertFuncletPrologBlock(BasicBlock* block);
Expand Down
49 changes: 1 addition & 48 deletions src/coreclr/jit/emit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1865,11 +1865,6 @@ void emitter::emitCheckIGList()
// Extension groups don't store GC info.
assert((currIG->igFlags & (IGF_GC_VARS | IGF_BYREF_REGS)) == 0);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
// Extension groups can't be branch targets.
assert((currIG->igFlags & IGF_FINALLY_TARGET) == 0);
#endif

// TODO: It would be nice if we could assert that a funclet prolog, funclet epilog, or
// function epilog could only extend one of the same type. However, epilogs are created
// using emitCreatePlaceholderIG() and might be in EXTEND groups. Can we force them to
Expand Down Expand Up @@ -2900,8 +2895,7 @@ bool emitter::emitNoGChelper(CORINFO_METHOD_HANDLE methHnd)

void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars,
regMaskTP gcrefRegs,
regMaskTP byrefRegs,
bool isFinallyTarget DEBUG_ARG(BasicBlock* block))
regMaskTP byrefRegs DEBUG_ARG(BasicBlock* block))
{
/* Create a new IG if the current one is non-empty */

Expand All @@ -2925,13 +2919,6 @@ void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars,
emitThisGCrefRegs = emitInitGCrefRegs = gcrefRegs;
emitThisByrefRegs = emitInitByrefRegs = byrefRegs;

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
if (isFinallyTarget)
{
emitCurIG->igFlags |= IGF_FINALLY_TARGET;
}
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)

#ifdef DEBUG
if (EMIT_GC_VERBOSE)
{
Expand Down Expand Up @@ -3997,12 +3984,6 @@ void emitter::emitDispIGflags(unsigned flags)
{
printf(", byref");
}
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
if (flags & IGF_FINALLY_TARGET)
{
printf(", ftarget");
}
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
if (flags & IGF_FUNCLET_PROLOG)
{
printf(", funclet prolog");
Expand Down Expand Up @@ -7125,11 +7106,6 @@ unsigned emitter::emitEndCodeGen(Compiler* comp,
NO_WAY("Too many instruction groups");
}

// If this instruction group is returned to from a funclet implementing a finally,
// on architectures where it is necessary generate GC info for the current instruction as
// if it were the instruction following a call.
emitGenGCInfoIfFuncletRetTarget(ig, cp);

instrDesc* id = emitFirstInstrDesc(ig->igData);

#ifdef DEBUG
Expand Down Expand Up @@ -7658,29 +7634,6 @@ unsigned emitter::emitEndCodeGen(Compiler* comp,
return actualCodeSize;
}

// See specification comment at the declaration.
void emitter::emitGenGCInfoIfFuncletRetTarget(insGroup* ig, BYTE* cp)
{
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
// We only emit this GC information on targets where finally's are implemented via funclets,
// and the finally is invoked, during non-exceptional execution, via a branch with a predefined
// link register, rather than a "true call" for which we would already generate GC info. Currently,
// this means precisely ARM.
if (ig->igFlags & IGF_FINALLY_TARGET)
{
// We don't actually have a call instruction in this case, so we don't have
// a real size for that instruction. We'll use 1.
emitStackPop(cp, /*isCall*/ true, /*callInstrSize*/ 1, /*args*/ 0);

/* Do we need to record a call location for GC purposes? */
if (!emitFullGCinfo)
{
emitRecordGCcall(cp, /*callInstrSize*/ 1);
}
}
#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
}

/*****************************************************************************
*
* We have an instruction in an insGroup and we need to know the
Expand Down
Loading