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.

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

JIT: minor inliner refactoring #44215

Merged
merged 2 commits into from
Nov 5, 2020
Merged

JIT: minor inliner refactoring #44215

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d3bf6b
JIT: minor inliner refactoring
AndyAyersMS Nov 3, 2020
34983a0
update header comment
AndyAyersMS Nov 4, 2020
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
42 changes: 42 additions & 0 deletions src/coreclr/src/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5508,6 +5508,12 @@ int Compiler::compCompile(CORINFO_MODULE_HANDLE classPtr,
}

#if defined(DEBUG) || defined(INLINE_DATA)
//------------------------------------------------------------------------
// compMethodHash: get hash code for currently jitted method
//
// Returns:
// Hash based on method's full name
//
unsigned Compiler::Info::compMethodHash() const
{
if (compMethodHashPrivate == 0)
Expand All @@ -5521,6 +5527,42 @@ unsigned Compiler::Info::compMethodHash() const
}
return compMethodHashPrivate;
}

//------------------------------------------------------------------------
// compMethodHash: get hash code for specified method
//
// Arguments:
// methodHnd - method of interest
//
// Returns:
// Hash based on method's full name
//
unsigned Compiler::compMethodHash(CORINFO_METHOD_HANDLE methodHnd)
{
// If this is the root method, delegate to the caching version
//
if (methodHnd == info.compMethodHnd)
{
return info.compMethodHash();
}

// Else compute from scratch. Might consider caching this too.
//
unsigned methodHash = 0;
const char* calleeName = eeGetMethodFullName(methodHnd);

if (calleeName != nullptr)
{
methodHash = HashStringA(calleeName);
}
else
{
methodHash = info.compCompHnd->getMethodHash(methodHnd);
}

return methodHash;
}

#endif // defined(DEBUG) || defined(INLINE_DATA)

void Compiler::compCompileFinish()
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7359,6 +7359,7 @@ class Compiler

const char* eeGetMethodName(CORINFO_METHOD_HANDLE hnd, const char** className);
const char* eeGetMethodFullName(CORINFO_METHOD_HANDLE hnd);
unsigned compMethodHash(CORINFO_METHOD_HANDLE methodHandle);

bool eeIsNativeMethod(CORINFO_METHOD_HANDLE method);
CORINFO_METHOD_HANDLE eeGetMethodHandleForNative(CORINFO_METHOD_HANDLE method);
Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/src/jit/inline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,9 +495,8 @@ void InlineContext::DumpXml(FILE* file, unsigned indent)
{
Compiler* compiler = m_InlineStrategy->GetCompiler();

mdMethodDef calleeToken = compiler->info.compCompHnd->getMethodDefFromMethod(m_Callee);
unsigned calleeHash = compiler->info.compCompHnd->getMethodHash(m_Callee);

mdMethodDef calleeToken = compiler->info.compCompHnd->getMethodDefFromMethod(m_Callee);
unsigned calleeHash = compiler->compMethodHash(m_Callee);
const char* inlineReason = InlGetObservationString(m_Observation);

int offset = -1;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/jit/inline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ class InlinePolicy

// Policy determinations
virtual void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) = 0;
virtual bool BudgetCheck() const = 0;

// Policy policies
virtual bool PropagateNeverToRuntime() const = 0;
Expand Down
108 changes: 71 additions & 37 deletions src/coreclr/src/jit/inlinepolicy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,35 +369,13 @@ void DefaultPolicy::NoteBool(InlineObservation obs, bool value)
// "reestablishes" candidacy rather than alters
// candidacy ... so instead we bail out here.
//
if (!m_IsPrejitRoot)
{
InlineStrategy* strategy = m_RootCompiler->m_inlineStrategy;
const bool overBudget = strategy->BudgetCheck(m_CodeSize);
bool overBudget = this->BudgetCheck();

if (overBudget)
{
// If the candidate is a forceinline and the callsite is
// not too deep, allow the inline even if it goes over budget.
//
// For now, "not too deep" means a top-level inline. Note
// depth 0 is used for the root method, so inline candidate depth
// will be 1 or more.
//
assert(m_IsForceInlineKnown);
assert(m_CallsiteDepth > 0);
const bool allowOverBudget = m_IsForceInline && (m_CallsiteDepth == 1);

if (allowOverBudget)
{
JITDUMP("Allowing over-budget top-level forceinline\n");
}
else
{
SetFailure(InlineObservation::CALLSITE_OVER_BUDGET);
}
}
if (overBudget)
{
SetFailure(InlineObservation::CALLSITE_OVER_BUDGET);
return;
}

break;
}

Expand Down Expand Up @@ -461,6 +439,53 @@ void DefaultPolicy::NoteBool(InlineObservation obs, bool value)
}
}

//------------------------------------------------------------------------
// BudgetCheck: see if this inline would exceed the current budget
//
// Returns:
// True if inline would exceed the budget.
//
bool DefaultPolicy::BudgetCheck() const
{
// Only relevant if we're actually inlining.
//
if (m_IsPrejitRoot)
{
return false;
}

// The strategy tracks the amout of inlining done so far,
// so it performs the actual check.
//
InlineStrategy* strategy = m_RootCompiler->m_inlineStrategy;
const bool overBudget = strategy->BudgetCheck(m_CodeSize);

if (overBudget)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If we will have more cases like this it could be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow -- does "more cases" refer to more overrides of the BudgetCheck, or more reasons we might allow inlines to go over budget?

I will probably adjust this bit of the default policy soon to address #41692.

Copy link
Contributor

Choose a reason for hiding this comment

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

More reasons and more overrides where we allow to go over budget when the header says True if inline would exceed the budget. and the user is const bool overBudget = this->BudgetCheck();

Copy link
Member Author

Choose a reason for hiding this comment

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

There's only ever going to be one reason to go over budget -- when we have an aggressive inline that we suspect might end up being less costly than it looks.

And likely no more overrides.

{
// If the candidate is a forceinline and the callsite is
// not too deep, allow the inline even if it goes over budget.
//
// For now, "not too deep" means a top-level inline. Note
// depth 0 is used for the root method, so inline candidate depth
// will be 1 or more.
//
assert(m_IsForceInlineKnown);
assert(m_CallsiteDepth > 0);
const bool allowOverBudget = m_IsForceInline && (m_CallsiteDepth == 1);

if (allowOverBudget)
{
JITDUMP("Allowing over-budget top-level forceinline\n");
}
else
{
return true;
}
}

return false;
}

//------------------------------------------------------------------------
// NoteInt: handle an observed integer value
//
Expand Down Expand Up @@ -1012,15 +1037,11 @@ void RandomPolicy::DetermineProfitability(CORINFO_METHOD_INFO* methodInfo)
assert(m_Observation == InlineObservation::CALLEE_IS_DISCRETIONARY_INLINE);

// Budget check.
if (!m_IsPrejitRoot)
const bool overBudget = this->BudgetCheck();
if (overBudget)
{
InlineStrategy* strategy = m_RootCompiler->m_inlineStrategy;
bool overBudget = strategy->BudgetCheck(m_CodeSize);
if (overBudget)
{
SetFailure(InlineObservation::CALLSITE_OVER_BUDGET);
return;
}
SetFailure(InlineObservation::CALLSITE_OVER_BUDGET);
return;
}

// If we're also dumping inline data, make additional observations
Expand Down Expand Up @@ -2179,6 +2200,19 @@ FullPolicy::FullPolicy(Compiler* compiler, bool isPrejitRoot) : DiscretionaryPol
// Empty
}

//------------------------------------------------------------------------
// BudgetCheck: see if this inline would exceed the current budget
//
// Returns:
// True if inline would exceed the budget.
//
bool FullPolicy::BudgetCheck() const
{
// There are no budget restrictions for the full policy.
//
return false;
}

//------------------------------------------------------------------------
// DetermineProfitability: determine if this inline is profitable
//
Expand Down Expand Up @@ -2481,7 +2515,7 @@ bool ReplayPolicy::FindContext(InlineContext* context)
//
// Token and Hash we're looking for.
mdMethodDef contextToken = m_RootCompiler->info.compCompHnd->getMethodDefFromMethod(context->GetCallee());
unsigned contextHash = m_RootCompiler->info.compCompHnd->getMethodHash(context->GetCallee());
unsigned contextHash = m_RootCompiler->compMethodHash(context->GetCallee());
unsigned contextOffset = (unsigned)context->GetOffset();

return FindInline(contextToken, contextHash, contextOffset);
Expand Down Expand Up @@ -2665,7 +2699,7 @@ bool ReplayPolicy::FindInline(CORINFO_METHOD_HANDLE callee)
{
// Token and Hash we're looking for
mdMethodDef calleeToken = m_RootCompiler->info.compCompHnd->getMethodDefFromMethod(callee);
unsigned calleeHash = m_RootCompiler->info.compCompHnd->getMethodHash(callee);
unsigned calleeHash = m_RootCompiler->compMethodHash(callee);

// Abstract this or just pass through raw bits
// See matching code in xml writer
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/jit/inlinepolicy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ class DefaultPolicy : public LegalPolicy

// Policy determinations
void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) override;
bool BudgetCheck() const override;

// Policy policies
bool PropagateNeverToRuntime() const override;
Expand Down Expand Up @@ -350,6 +351,7 @@ class FullPolicy : public DiscretionaryPolicy

// Policy determinations
void DetermineProfitability(CORINFO_METHOD_INFO* methodInfo) override;
bool BudgetCheck() const override;

// Miscellaneous
const char* GetName() const override
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/src/jit/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,14 @@ void ConfigMethodRange::Dump()
printf("<method range with %d entries>\n", m_lastRange);
for (unsigned i = 0; i < m_lastRange; i++)
{
printf("%i [%u-%u]\n", i, m_ranges[i].m_low, m_ranges[i].m_high);
if (m_ranges[i].m_low == m_ranges[i].m_high)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: While you are here could you please update the header and delete hash -- hash value to check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

{
printf("%i [0x%08x]\n", i, m_ranges[i].m_low);
}
else
{
printf("%i [0x%08x-0x%08x]\n", i, m_ranges[i].m_low, m_ranges[i].m_high);
}
}
}

Expand Down