From 5806265fc2cd3f7b0e4225e4497dd937e2ca9333 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 2 Nov 2017 12:56:39 -0700 Subject: [PATCH] JIT: make suitably optimistic prejit inline assessments When prejitting, the jit assesses whether each root method is a potential inline candidate for any possible caller. Methods deemed un-inlinable in any caller are marked in the runtime as "noinline" to save the jit some work later on when it sees calls to these methods. This assessment was too conservative and led to prejit-ordering dependences for inlines. It also meant that prejitting was missing some inlines that would have happened had we not done the prejit root assessment. This change removes some of the prejit observation blockers. These mostly will enable more prejit methods to become candidates. We also now track when a method argument reaches a test. When we are assessing profitability for a prejit root, assume the call site best possible case. Also, update the inline XML to capture the prejit assessments. This increases the number of inlines considered and performed when prejitting and will also lead to slightly more inlining when jitting. However we do not expect a large througput impact when jitting -- the main impact of this change is that inlining when prejitting is now just as aggressive as inlining when jitting, and the decisions no longer depend on the order in which methods are prejitted. Closes #14441. Closes #3482. --- src/jit/compiler.cpp | 2 ++ src/jit/flowgraph.cpp | 4 +++ src/jit/inline.cpp | 19 +++++++++++++- src/jit/inline.def | 1 + src/jit/inline.h | 51 +++++++++++++++++++++--------------- src/jit/inlinepolicy.cpp | 55 +++++++++++---------------------------- src/jit/inlinepolicy.h | 2 ++ src/jit/jitconfigvalues.h | 5 ++-- 8 files changed, 75 insertions(+), 64 deletions(-) diff --git a/src/jit/compiler.cpp b/src/jit/compiler.cpp index efad4cfd0f05..3147eb15059b 100644 --- a/src/jit/compiler.cpp +++ b/src/jit/compiler.cpp @@ -6074,6 +6074,8 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr, prejitResult.DetermineProfitability(methodInfo); } + m_inlineStrategy->NotePrejitDecision(prejitResult); + // Handle the results of the inline analysis. if (prejitResult.IsFailure()) { diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 15f0d5d53c38..4e312f596e78 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -5107,6 +5107,8 @@ void Compiler::fgObserveInlineConstants(OPCODE opcode, const FgStack& stack, boo { if (FgStack::IsArgument(slot0)) { + compInlineResult->Note(InlineObservation::CALLEE_ARG_FEEDS_TEST); + unsigned varNum = FgStack::SlotTypeToArgNum(slot0); if (impInlineInfo->inlArgInfo[varNum].argNode->OperIsConst()) { @@ -5116,6 +5118,8 @@ void Compiler::fgObserveInlineConstants(OPCODE opcode, const FgStack& stack, boo if (FgStack::IsArgument(slot1)) { + compInlineResult->Note(InlineObservation::CALLEE_ARG_FEEDS_TEST); + unsigned varNum = FgStack::SlotTypeToArgNum(slot1); if (impInlineInfo->inlArgInfo[varNum].argNode->OperIsConst()) { diff --git a/src/jit/inline.cpp b/src/jit/inline.cpp index 05fcf1c6b97b..679ff4124000 100644 --- a/src/jit/inline.cpp +++ b/src/jit/inline.cpp @@ -474,6 +474,12 @@ void InlineContext::DumpXml(FILE* file, unsigned indent) m_Sibling->DumpXml(file, indent); } + // Optionally suppress failing inline records + if ((JitConfig.JitInlineDumpXml() == 3) && !m_Success) + { + return; + } + const bool isRoot = m_Parent == nullptr; const bool hasChild = m_Child != nullptr; const char* inlineType = m_Success ? "Inline" : "FailedInline"; @@ -739,6 +745,8 @@ InlineStrategy::InlineStrategy(Compiler* compiler) : m_Compiler(compiler) , m_RootContext(nullptr) , m_LastSuccessfulPolicy(nullptr) + , m_LastContext(nullptr) + , m_PrejitRootDecision(InlineDecision::UNDECIDED) , m_CallCount(0) , m_CandidateCount(0) , m_AlwaysCandidateCount(0) @@ -1493,7 +1501,7 @@ void InlineStrategy::DumpXml(FILE* file, unsigned indent) // If we're dumping "minimal" Xml, and we didn't do // any inlines into this method, then there's nothing // to emit here. - if ((m_InlineCount == 0) && (JitConfig.JitInlineDumpXml() == 2)) + if ((m_InlineCount == 0) && (JitConfig.JitInlineDumpXml() >= 2)) { return; } @@ -1566,6 +1574,15 @@ void InlineStrategy::DumpXml(FILE* file, unsigned indent) fprintf(file, "%*s%u\n", indent + 2, "", m_CurrentSizeEstimate / 10); fprintf(file, "%*s%u\n", indent + 2, "", m_CurrentTimeEstimate); + // For prejit roots also propagate out the assessment of the root method + if (isPrejitRoot) + { + fprintf(file, "%*s%s\n", indent + 2, "", + InlGetDecisionString(m_PrejitRootDecision)); + fprintf(file, "%*s%s\n", indent + 2, "", + InlGetObservationString(m_PrejitRootObservation)); + } + // Root context will be null if we're not optimizing the method. // // Note there are cases of this in mscorlib even in release builds, diff --git a/src/jit/inline.def b/src/jit/inline.def index 0a13950b4843..4dd67402fb7f 100644 --- a/src/jit/inline.def +++ b/src/jit/inline.def @@ -72,6 +72,7 @@ INLINE_OBSERVATION(TOO_MUCH_IL, bool, "too many il bytes", // ------ Callee Information ------- INLINE_OBSERVATION(ARG_FEEDS_CONSTANT_TEST, bool, "argument feeds constant test", INFORMATION, CALLEE) +INLINE_OBSERVATION(ARG_FEEDS_TEST, bool, "argument feeds test", INFORMATION, CALLEE) INLINE_OBSERVATION(ARG_FEEDS_RANGE_CHECK, bool, "argument feeds range check", INFORMATION, CALLEE) INLINE_OBSERVATION(BEGIN_OPCODE_SCAN, bool, "prepare to look at opcodes", INFORMATION, CALLEE) INLINE_OBSERVATION(BELOW_ALWAYS_INLINE_SIZE, bool, "below ALWAYS_INLINE size", INFORMATION, CALLEE) diff --git a/src/jit/inline.h b/src/jit/inline.h index f06b4f7a6f21..757efec6a552 100644 --- a/src/jit/inline.h +++ b/src/jit/inline.h @@ -782,6 +782,13 @@ class InlineStrategy m_ImportCount++; } + // Inform strategy about the inline decision for a prejit root + void NotePrejitDecision(const InlineResult& r) + { + m_PrejitRootDecision = r.GetPolicy()->GetDecision(); + m_PrejitRootObservation = r.GetPolicy()->GetObservation(); + } + // Dump csv header for inline stats to indicated file. static void DumpCsvHeader(FILE* f); @@ -866,27 +873,29 @@ class InlineStrategy static CritSecObject s_XmlWriterLock; #endif // defined(DEBUG) || defined(INLINE_DATA) - Compiler* m_Compiler; - InlineContext* m_RootContext; - InlinePolicy* m_LastSuccessfulPolicy; - InlineContext* m_LastContext; - unsigned m_CallCount; - unsigned m_CandidateCount; - unsigned m_AlwaysCandidateCount; - unsigned m_ForceCandidateCount; - unsigned m_DiscretionaryCandidateCount; - unsigned m_UnprofitableCandidateCount; - unsigned m_ImportCount; - unsigned m_InlineCount; - unsigned m_MaxInlineSize; - unsigned m_MaxInlineDepth; - int m_InitialTimeBudget; - int m_InitialTimeEstimate; - int m_CurrentTimeBudget; - int m_CurrentTimeEstimate; - int m_InitialSizeEstimate; - int m_CurrentSizeEstimate; - bool m_HasForceViaDiscretionary; + Compiler* m_Compiler; + InlineContext* m_RootContext; + InlinePolicy* m_LastSuccessfulPolicy; + InlineContext* m_LastContext; + InlineDecision m_PrejitRootDecision; + InlineObservation m_PrejitRootObservation; + unsigned m_CallCount; + unsigned m_CandidateCount; + unsigned m_AlwaysCandidateCount; + unsigned m_ForceCandidateCount; + unsigned m_DiscretionaryCandidateCount; + unsigned m_UnprofitableCandidateCount; + unsigned m_ImportCount; + unsigned m_InlineCount; + unsigned m_MaxInlineSize; + unsigned m_MaxInlineDepth; + int m_InitialTimeBudget; + int m_InitialTimeEstimate; + int m_CurrentTimeBudget; + int m_CurrentTimeEstimate; + int m_InitialSizeEstimate; + int m_CurrentSizeEstimate; + bool m_HasForceViaDiscretionary; #if defined(DEBUG) || defined(INLINE_DATA) long m_MethodXmlFilePosition; diff --git a/src/jit/inlinepolicy.cpp b/src/jit/inlinepolicy.cpp index 5847bbcb5460..eaf739612c45 100644 --- a/src/jit/inlinepolicy.cpp +++ b/src/jit/inlinepolicy.cpp @@ -267,37 +267,24 @@ void DefaultPolicy::NoteBool(InlineObservation obs, bool value) break; case InlineObservation::CALLEE_LOOKS_LIKE_WRAPPER: - // DefaultPolicy ignores this for prejit roots. - if (!m_IsPrejitRoot) - { - m_LooksLikeWrapperMethod = value; - } + m_LooksLikeWrapperMethod = value; + break; + + case InlineObservation::CALLEE_ARG_FEEDS_TEST: + m_ArgFeedsTest++; break; case InlineObservation::CALLEE_ARG_FEEDS_CONSTANT_TEST: - // DefaultPolicy ignores this for prejit roots. - if (!m_IsPrejitRoot) - { - m_ArgFeedsConstantTest++; - } + m_ArgFeedsConstantTest++; break; case InlineObservation::CALLEE_ARG_FEEDS_RANGE_CHECK: - // DefaultPolicy ignores this for prejit roots. - if (!m_IsPrejitRoot) - { - m_ArgFeedsRangeCheck++; - } + m_ArgFeedsRangeCheck++; break; case InlineObservation::CALLEE_HAS_SWITCH: case InlineObservation::CALLEE_UNSUPPORTED_OPCODE: - // DefaultPolicy ignores these for prejit roots. - if (!m_IsPrejitRoot) - { - // Pass these on, they should cause inlining to fail. - propagate = true; - } + propagate = true; break; case InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST: @@ -649,6 +636,13 @@ double DefaultPolicy::DetermineMultiplier() multiplier += 3.0; JITDUMP("\nInline candidate has const arg that feeds a conditional. Multiplier increased to %g.", multiplier); } + // For prejit roots we do not see the call sites. To be suitably optimisitic + // assume that call sites may pass constants. + else if (m_IsPrejitRoot && ((m_ArgFeedsConstantTest > 0) || (m_ArgFeedsTest > 0))) + { + multiplier += 3.0; + JITDUMP("\nPrejit root candidate has arg that feeds a conditional. Multiplier increased to %g.", multiplier); + } switch (m_CallsiteFrequency) { @@ -1158,25 +1152,6 @@ void DiscretionaryPolicy::NoteBool(InlineObservation obs, bool value) { switch (obs) { - case InlineObservation::CALLEE_LOOKS_LIKE_WRAPPER: - m_LooksLikeWrapperMethod = value; - break; - - case InlineObservation::CALLEE_ARG_FEEDS_CONSTANT_TEST: - assert(value); - m_ArgFeedsConstantTest++; - break; - - case InlineObservation::CALLEE_ARG_FEEDS_RANGE_CHECK: - assert(value); - m_ArgFeedsRangeCheck++; - break; - - case InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST: - assert(value); - m_ConstantArgFeedsConstantTest++; - break; - case InlineObservation::CALLEE_IS_CLASS_CTOR: m_IsClassCtor = value; break; diff --git a/src/jit/inlinepolicy.h b/src/jit/inlinepolicy.h index 0ff0b3327996..b1094bbaaa0b 100644 --- a/src/jit/inlinepolicy.h +++ b/src/jit/inlinepolicy.h @@ -84,6 +84,7 @@ class DefaultPolicy : public LegalPolicy , m_CallsiteFrequency(InlineCallsiteFrequency::UNUSED) , m_InstructionCount(0) , m_LoadStoreCount(0) + , m_ArgFeedsTest(0) , m_ArgFeedsConstantTest(0) , m_ArgFeedsRangeCheck(0) , m_ConstantArgFeedsConstantTest(0) @@ -148,6 +149,7 @@ class DefaultPolicy : public LegalPolicy InlineCallsiteFrequency m_CallsiteFrequency; unsigned m_InstructionCount; unsigned m_LoadStoreCount; + unsigned m_ArgFeedsTest; unsigned m_ArgFeedsConstantTest; unsigned m_ArgFeedsRangeCheck; unsigned m_ConstantArgFeedsConstantTest; diff --git a/src/jit/jitconfigvalues.h b/src/jit/jitconfigvalues.h index bb764f4e506c..e9b9f0d27515 100644 --- a/src/jit/jitconfigvalues.h +++ b/src/jit/jitconfigvalues.h @@ -310,8 +310,9 @@ CONFIG_STRING(JitMeasureNowayAssertFile, #if defined(DEBUG) || defined(INLINE_DATA) CONFIG_INTEGER(JitInlineDumpData, W("JitInlineDumpData"), 0) -CONFIG_INTEGER(JitInlineDumpXml, W("JitInlineDumpXml"), 0) // 1 = full xml (all methods), 2 = minimal xml (only method - // with inlines) +CONFIG_INTEGER(JitInlineDumpXml, W("JitInlineDumpXml"), 0) // 1 = full xml (+ failures in DEBUG) + // 2 = only methods with inlines (+ failures in DEBUG) + // 3 = only methods with inlines, no failures CONFIG_INTEGER(JitInlineLimit, W("JitInlineLimit"), -1) CONFIG_INTEGER(JitInlinePolicyDiscretionary, W("JitInlinePolicyDiscretionary"), 0) CONFIG_INTEGER(JitInlinePolicyFull, W("JitInlinePolicyFull"), 0)