From 3d3bf6bbe6283159375ddeff2b3cc801867a2fda Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 3 Nov 2020 10:43:37 -0800 Subject: [PATCH 1/2] JIT: minor inliner refactoring Extract out the budget check logic so it can vary by inlinling policy. Use this to exempt the FullPolicy from budget checking. Fix inline xml to dump the proper (full name) hash for inlinees. Update range dumper to dump ranges in hex. --- src/coreclr/src/jit/compiler.cpp | 42 +++++++++++ src/coreclr/src/jit/compiler.h | 1 + src/coreclr/src/jit/inline.cpp | 5 +- src/coreclr/src/jit/inline.h | 1 + src/coreclr/src/jit/inlinepolicy.cpp | 108 ++++++++++++++++++--------- src/coreclr/src/jit/inlinepolicy.h | 2 + src/coreclr/src/jit/utils.cpp | 9 ++- 7 files changed, 127 insertions(+), 41 deletions(-) diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index ebfe8e3dc5455..4c0d72250311c 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -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) @@ -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() diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index b5d3249923e10..c0e537acd2bce 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -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); diff --git a/src/coreclr/src/jit/inline.cpp b/src/coreclr/src/jit/inline.cpp index 759a6e3661812..050214e188b2e 100644 --- a/src/coreclr/src/jit/inline.cpp +++ b/src/coreclr/src/jit/inline.cpp @@ -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; diff --git a/src/coreclr/src/jit/inline.h b/src/coreclr/src/jit/inline.h index 8302356c6e8fd..b2c414d137055 100644 --- a/src/coreclr/src/jit/inline.h +++ b/src/coreclr/src/jit/inline.h @@ -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; diff --git a/src/coreclr/src/jit/inlinepolicy.cpp b/src/coreclr/src/jit/inlinepolicy.cpp index 09a029f996638..0502515cba16c 100644 --- a/src/coreclr/src/jit/inlinepolicy.cpp +++ b/src/coreclr/src/jit/inlinepolicy.cpp @@ -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; } @@ -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) + { + // 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 // @@ -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 @@ -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 // @@ -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); @@ -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 diff --git a/src/coreclr/src/jit/inlinepolicy.h b/src/coreclr/src/jit/inlinepolicy.h index 9024996521a3a..7bd6557b2511c 100644 --- a/src/coreclr/src/jit/inlinepolicy.h +++ b/src/coreclr/src/jit/inlinepolicy.h @@ -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; @@ -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 diff --git a/src/coreclr/src/jit/utils.cpp b/src/coreclr/src/jit/utils.cpp index de1fd4a880b2c..d7eb7ae344ad1 100644 --- a/src/coreclr/src/jit/utils.cpp +++ b/src/coreclr/src/jit/utils.cpp @@ -866,7 +866,14 @@ void ConfigMethodRange::Dump() printf("\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) + { + 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); + } } } From 34983a01819760535fcca6ef99f5770c15864e26 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 4 Nov 2020 14:59:05 -0800 Subject: [PATCH 2/2] update header comment --- src/coreclr/src/jit/utils.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/src/jit/utils.cpp b/src/coreclr/src/jit/utils.cpp index d7eb7ae344ad1..133b49f701a12 100644 --- a/src/coreclr/src/jit/utils.cpp +++ b/src/coreclr/src/jit/utils.cpp @@ -846,9 +846,6 @@ void ConfigMethodRange::InitRanges(const WCHAR* rangeStr, unsigned capacity) //------------------------------------------------------------------------ // Dump: dump hash ranges to stdout // -// Arguments: -// hash -- hash value to check - void ConfigMethodRange::Dump() { if (m_inited != 1)