Skip to content

Commit

Permalink
JIT: removed unneeded throw helper blocks
Browse files Browse the repository at this point in the history
Various optimizations can happen between the time the throw helper blocks
are first requested (morph) and finally used (codegen). Prune away ones
that aren't needed during the stack level setter.

Fixes dotnet#93948.
  • Loading branch information
AndyAyersMS committed Nov 29, 2023
1 parent b190ea8 commit c995902
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 19 deletions.
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -6295,6 +6295,7 @@ class Compiler
BasicBlock* acdDstBlk; // block to which we jump
unsigned acdData;
SpecialCodeKind acdKind; // what kind of a special block is this?
bool acdUsed; // do we need to keep this helper block?
#if !FEATURE_FIXED_OUT_ARGS
bool acdStkLvlInit; // has acdStkLvl value been already set?
unsigned acdStkLvl; // stack level in stack slots.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/flowgraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3568,6 +3568,10 @@ void Compiler::fgAddCodeRef(BasicBlock* srcBlk, SpecialCodeKind kind)
add->acdStkLvlInit = false;
#endif // !FEATURE_FIXED_OUT_ARGS

// This gets set true in the stack level setter
// if there's still a need for this helper
add->acdUsed = false;

fgAddCodeList = add;

// Defer creating of the blocks until later.
Expand Down
40 changes: 27 additions & 13 deletions src/coreclr/jit/stacklevelsetter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ StackLevelSetter::StackLevelSetter(Compiler* compiler)
, maxStackLevel(0)
, memAllocator(compiler->getAllocator(CMK_CallArgs))
, putArgNumSlots(memAllocator)
, throwHelperBlocksUsed(comp->fgUseThrowHelperBlocks() && comp->compUsesThrowHelper)
#if !FEATURE_FIXED_OUT_ARGS
, framePointerRequired(compiler->codeGen->isFramePointerRequired())
, throwHelperBlocksUsed(comp->fgUseThrowHelperBlocks() && comp->compUsesThrowHelper)
#endif // !FEATURE_FIXED_OUT_ARGS
{
// The constructor reads this value to skip iterations that could set it if it is already set.
Expand Down Expand Up @@ -59,8 +59,25 @@ PhaseStatus StackLevelSetter::DoPhase()
comp->fgSetPtrArgCntMax(maxStackLevel);
CheckArgCnt();

// Check if there are any unused throw helper blocks, and if so, remove them.
bool madeChanges = false;

for (Compiler::AddCodeDsc* add = comp->fgGetAdditionalCodeDescriptors(); add != nullptr; add = add->acdNext)
{
if (add->acdUsed)
{
continue;
}

BasicBlock* const block = add->acdDstBlk;
JITDUMP("Throw help block " FMT_BB " is unused\n", block->bbNum);
block->bbFlags &= ~BBF_DONT_REMOVE;
comp->fgRemoveBlock(block, /* unreachable */ true);
madeChanges = true;
}

// Might want an "other" category for things like this...
return PhaseStatus::MODIFIED_NOTHING;
return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING;
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -89,16 +106,10 @@ void StackLevelSetter::ProcessBlock(BasicBlock* block)
SubStackLevel(numSlots);
}

#if !FEATURE_FIXED_OUT_ARGS
// Set throw blocks incoming stack depth for x86.
if (throwHelperBlocksUsed && !framePointerRequired)
if (throwHelperBlocksUsed && node->OperMayThrow(comp))
{
if (node->OperMayThrow(comp))
{
SetThrowHelperBlocks(node, block);
}
SetThrowHelperBlocks(node, block);
}
#endif // !FEATURE_FIXED_OUT_ARGS

if (node->IsCall())
{
Expand All @@ -112,7 +123,6 @@ void StackLevelSetter::ProcessBlock(BasicBlock* block)
assert(currentStackLevel == 0);
}

#if !FEATURE_FIXED_OUT_ARGS
//------------------------------------------------------------------------
// SetThrowHelperBlocks: Set throw helper blocks incoming stack levels targeted
// from the node.
Expand Down Expand Up @@ -171,6 +181,11 @@ void StackLevelSetter::SetThrowHelperBlock(SpecialCodeKind kind, BasicBlock* blo
{
Compiler::AddCodeDsc* add = comp->fgFindExcptnTarget(kind, comp->bbThrowIndex(block));
assert(add != nullptr);
// We expect we'll actually need this helper.
add->acdUsed = true;

#if !FEATURE_FIXED_OUT_ARGS

if (add->acdStkLvlInit)
{
// If different range checks happen at different stack levels,
Expand Down Expand Up @@ -217,9 +232,8 @@ void StackLevelSetter::SetThrowHelperBlock(SpecialCodeKind kind, BasicBlock* blo
#endif // Debug
add->acdStkLvl = currentStackLevel;
}
}

#endif // !FEATURE_FIXED_OUT_ARGS
}

//------------------------------------------------------------------------
// PopArgumentsFromCall: Calculate the number of stack arguments that are used by the call.
Expand Down
10 changes: 4 additions & 6 deletions src/coreclr/jit/stacklevelsetter.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@ class StackLevelSetter final : public Phase
private:
void ProcessBlock(BasicBlock* block);

#if !FEATURE_FIXED_OUT_ARGS
void SetThrowHelperBlocks(GenTree* node, BasicBlock* block);
void SetThrowHelperBlock(SpecialCodeKind kind, BasicBlock* block);
#endif // !FEATURE_FIXED_OUT_ARGS

unsigned PopArgumentsFromCall(GenTreeCall* call);
void AddStackLevel(unsigned value);
Expand All @@ -35,10 +33,10 @@ class StackLevelSetter final : public Phase
CompAllocator memAllocator;

typedef JitHashTable<GenTreePutArgStk*, JitPtrKeyFuncs<GenTreePutArgStk>, unsigned> PutArgNumSlotsMap;
PutArgNumSlotsMap putArgNumSlots; // The hash table keeps stack slot sizes for active GT_PUTARG_STK nodes.
PutArgNumSlotsMap putArgNumSlots; // The hash table keeps stack slot sizes for active GT_PUTARG_STK nodes.
bool throwHelperBlocksUsed; // Were any throw helper blocks requested for this method.

#if !FEATURE_FIXED_OUT_ARGS
bool framePointerRequired; // Is frame pointer required based on the analysis made by this phase.
bool throwHelperBlocksUsed; // Were any throw helper blocks created for this method.
#endif // !FEATURE_FIXED_OUT_ARGS
bool framePointerRequired; // Is frame pointer required based on the analysis made by this phase.
#endif // !FEATURE_FIXED_OUT_ARGS
};

0 comments on commit c995902

Please sign in to comment.