From 23cc05e12713472bdf8a782f2e695d64da6f4c88 Mon Sep 17 00:00:00 2001 From: EgorBo <egorbo@gmail.com> Date: Mon, 5 Jun 2023 01:35:05 +0200 Subject: [PATCH 1/2] Fix inline decision reporting + noinline propagation --- src/coreclr/jit/importercalls.cpp | 19 +++++++++++-------- src/coreclr/jit/inline.def | 2 -- src/coreclr/jit/inline.h | 6 +++--- src/coreclr/jit/inlinepolicy.cpp | 17 +++++++++++++++++ 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 4afb8c55535495..150022c03dd129 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6280,12 +6280,6 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { if (!opts.OptEnabled(CLFLG_INLINING)) { - /* XXX Mon 8/18/2008 - * This assert is misleading. The caller does not ensure that we have CLFLG_INLINING set before - * calling impMarkInlineCandidate. However, if this assert trips it means that we're an inlinee and - * CLFLG_MINOPT is set. That doesn't make a lot of sense. If you hit this assert, work back and - * figure out why we did not set MAXOPT for this compile. - */ assert(!compIsForInlining()); return; } @@ -6301,11 +6295,12 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, assert(call->GetInlineCandidatesCount() > 0); for (uint8_t candidateId = 0; candidateId < call->GetInlineCandidatesCount(); candidateId++) { - InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate", true); + InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate for GDV"); // Do the actual evaluation impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); + inlineResult.Report(); // Ignore non-inlineable candidates // TODO: Consider keeping them to just devirtualize without inlining, at least for interface @@ -6316,14 +6311,22 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, candidateId--; } } + + // None of the candidates made it, make sure the call is no longer marked as "has inline info" + if (call->GetInlineCandidatesCount() == 0) + { + assert(!call->IsInlineCandidate()); + assert(!call->IsGuardedDevirtualizationCandidate()); + } } else { const uint8_t candidatesCount = call->GetInlineCandidatesCount(); assert(candidatesCount <= 1); - InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate", true); + InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); + inlineResult.Report(); } // If this call is an inline candidate or is not a guarded devirtualization diff --git a/src/coreclr/jit/inline.def b/src/coreclr/jit/inline.def index b918fb61282dd0..9371e655bde1ee 100644 --- a/src/coreclr/jit/inline.def +++ b/src/coreclr/jit/inline.def @@ -28,7 +28,6 @@ INLINE_OBSERVATION(UNUSED_INITIAL, bool, "unused initial observatio INLINE_OBSERVATION(BAD_ARGUMENT_NUMBER, bool, "invalid argument number", FATAL, CALLEE) INLINE_OBSERVATION(BAD_LOCAL_NUMBER, bool, "invalid local number", FATAL, CALLEE) INLINE_OBSERVATION(COMPILATION_ERROR, bool, "compilation error", FATAL, CALLEE) -INLINE_OBSERVATION(EXCEEDS_THRESHOLD, bool, "exceeds profit threshold", FATAL, CALLEE) INLINE_OBSERVATION(HAS_EH, bool, "has exception handling", FATAL, CALLEE) INLINE_OBSERVATION(HAS_ENDFILTER, bool, "has endfilter", FATAL, CALLEE) INLINE_OBSERVATION(HAS_ENDFINALLY, bool, "has endfinally", FATAL, CALLEE) @@ -132,7 +131,6 @@ INLINE_OBSERVATION(CANT_EMBED_VARARGS_COOKIE, bool, "can't embed varargs cooki INLINE_OBSERVATION(CANT_CLASS_INIT, bool, "can't class init", FATAL, CALLSITE) INLINE_OBSERVATION(COMPILATION_ERROR, bool, "compilation error", FATAL, CALLSITE) INLINE_OBSERVATION(COMPILATION_FAILURE, bool, "failed to compile", FATAL, CALLSITE) -INLINE_OBSERVATION(EXCEEDS_THRESHOLD, bool, "exceeds profit threshold", FATAL, CALLSITE) INLINE_OBSERVATION(EXPLICIT_TAIL_PREFIX, bool, "explicit tail prefix", FATAL, CALLSITE) INLINE_OBSERVATION(GENERIC_DICTIONARY_LOOKUP, bool, "runtime dictionary lookup", FATAL, CALLSITE) INLINE_OBSERVATION(HAS_CALL_VIA_LDVIRTFTN, bool, "call via ldvirtftn", FATAL, CALLSITE) diff --git a/src/coreclr/jit/inline.h b/src/coreclr/jit/inline.h index e350393cfd29c0..fe5f99d6f1a076 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -556,14 +556,14 @@ class InlineResult m_ImportedILSize = x; } + // Report/log/dump decision as appropriate + void Report(); + private: // No copying or assignment allowed. InlineResult(const InlineResult&) = delete; InlineResult& operator=(const InlineResult&) = delete; - // Report/log/dump decision as appropriate - void Report(); - Compiler* m_RootCompiler; InlinePolicy* m_Policy; GenTreeCall* m_Call; diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 75088eb0d8f471..433befab6995f1 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -2335,10 +2335,27 @@ bool DiscretionaryPolicy::PropagateNeverToRuntime() const // switch (m_Observation) { + // Not-profitable depends on call-site: case InlineObservation::CALLEE_NOT_PROFITABLE_INLINE: + return false; + + // If we mark no-returns as noinline we won't be able to recognize them + // as no-returns in future inlines. case InlineObservation::CALLEE_DOES_NOT_RETURN: return false; + // These also depend on call-sites + case InlineObservation::CALLSITE_OVER_BUDGET: + case InlineObservation::CALLSITE_TOO_MANY_LOCALS: + return false; + + // These thresholds may depend on PGO data (we allow more blocks/bigger IL size + // for hot methods so we want to make sure we won't bake 'noinline' just because + // some semi-hot callsite didn't expand these thresholds). + case InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS: + case InlineObservation::CALLEE_TOO_MUCH_IL: + return false; + default: return true; } From 8d3ae8320bd3a197728d1ad8ac31e8c2cb7e2e14 Mon Sep 17 00:00:00 2001 From: EgorBo <egorbo@gmail.com> Date: Wed, 7 Jun 2023 01:06:23 +0200 Subject: [PATCH 2/2] Address feedback --- src/coreclr/jit/importercalls.cpp | 3 --- src/coreclr/jit/inline.h | 6 +++--- src/coreclr/jit/inlinepolicy.cpp | 12 ------------ 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 150022c03dd129..86d91fa9b4a8e6 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6300,8 +6300,6 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, // Do the actual evaluation impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); - inlineResult.Report(); - // Ignore non-inlineable candidates // TODO: Consider keeping them to just devirtualize without inlining, at least for interface // calls on NativeAOT, but that requires more changes elsewhere too. @@ -6326,7 +6324,6 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); - inlineResult.Report(); } // If this call is an inline candidate or is not a guarded devirtualization diff --git a/src/coreclr/jit/inline.h b/src/coreclr/jit/inline.h index fe5f99d6f1a076..e350393cfd29c0 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -556,14 +556,14 @@ class InlineResult m_ImportedILSize = x; } - // Report/log/dump decision as appropriate - void Report(); - private: // No copying or assignment allowed. InlineResult(const InlineResult&) = delete; InlineResult& operator=(const InlineResult&) = delete; + // Report/log/dump decision as appropriate + void Report(); + Compiler* m_RootCompiler; InlinePolicy* m_Policy; GenTreeCall* m_Call; diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 433befab6995f1..73394192e25de1 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -2344,18 +2344,6 @@ bool DiscretionaryPolicy::PropagateNeverToRuntime() const case InlineObservation::CALLEE_DOES_NOT_RETURN: return false; - // These also depend on call-sites - case InlineObservation::CALLSITE_OVER_BUDGET: - case InlineObservation::CALLSITE_TOO_MANY_LOCALS: - return false; - - // These thresholds may depend on PGO data (we allow more blocks/bigger IL size - // for hot methods so we want to make sure we won't bake 'noinline' just because - // some semi-hot callsite didn't expand these thresholds). - case InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS: - case InlineObservation::CALLEE_TOO_MUCH_IL: - return false; - default: return true; }