Skip to content
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.

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

JIT: removed unneeded throw helper blocks #95379

Merged
merged 7 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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
60 changes: 46 additions & 14 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,24 @@ PhaseStatus StackLevelSetter::DoPhase()
comp->fgSetPtrArgCntMax(maxStackLevel);
CheckArgCnt();

// Might want an "other" category for things like this...
return PhaseStatus::MODIFIED_NOTHING;
// 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;
}

return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING;
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -89,16 +105,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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since GTF_CALL and GTF_EXCEPT are up-to-date and overapproximations here you can optimize TP with this trick:

if (((*use)->gtFlags & (GTF_EXCEPT | GTF_CALL)) == 0)
{
assert(!(*use)->OperMayThrow(m_compiler));
return use;
}
if (!(*use)->OperMayThrow(m_compiler))
{
return use;
}

{
if (node->OperMayThrow(comp))
{
SetThrowHelperBlocks(node, block);
}
SetThrowHelperBlocks(node, block);
}
#endif // !FEATURE_FIXED_OUT_ARGS

if (node->IsCall())
{
Expand All @@ -112,7 +122,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 @@ -147,6 +156,25 @@ void StackLevelSetter::SetThrowHelperBlocks(GenTree* node, BasicBlock* block)
SetThrowHelperBlock(SCK_ARITH_EXCPN, block);
break;

#if defined(TARGET_ARM64) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
case GT_DIV:
case GT_UDIV:
{
ExceptionSetFlags exSetFlags = node->OperExceptions(comp);

if ((exSetFlags & ExceptionSetFlags::DivideByZeroException) != ExceptionSetFlags::None)
{
SetThrowHelperBlock(SCK_DIV_BY_ZERO, block);
}

if ((exSetFlags & ExceptionSetFlags::ArithmeticException) != ExceptionSetFlags::None)
{
SetThrowHelperBlock(SCK_ARITH_EXCPN, block);
}
}
break;
#endif

default: // Other opers can target throw only due to overflow.
break;
}
Expand All @@ -171,6 +199,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 +250,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
};