From 8bc6d2bb7007a172c109f2362b42f23ad723c1bb Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 28 Apr 2025 08:59:16 -0700 Subject: [PATCH 1/2] JIT: use root compiler instance for sufficient PGO observation During inlining, we evaluate some aspects of an inlinee's viability while importing its direct caller. If that caller is not the inline root we may make inconsistent observations of the overall state of PGO. So for PGO observations always consult the root compiler. For example, the root R may have decided to inline a small method A that did not have PGO (say because of minimal profiling or lack of PGO for always inlined R2R methods), and that method calls another method B; we want to evaluate the viability of B using the PGO state of R, not of A. --- src/coreclr/jit/fginline.cpp | 11 +++++++++++ src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/inlinepolicy.cpp | 8 ++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index e6be97f1d38e74..813618b5319249 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -781,6 +781,17 @@ PhaseStatus Compiler::fgInline() Metrics.ProfileConsistentBeforeInline++; } + if (!fgHaveProfileWeights()) + { + JITDUMP("INLINER: no pgo data\n"); + } + else + { + JITDUMP("INLINER: pgo source is %s; pgo data is %sconsistent; %strusted; %ssufficient\n", + compGetPgoSourceName(), fgPgoConsistent ? "" : "not ", fgHaveTrustedProfileWeights() ? "" : "not ", + fgHaveSufficientProfileWeights() ? "" : "not "); + } + noway_assert(fgFirstBB != nullptr); BasicBlock* block = fgFirstBB; diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index ae3989f2f2d22c..9770f8f432604f 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -9256,7 +9256,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, // Profile data allows us to avoid early "too many IL bytes" outs. // inlineResult->NoteBool(InlineObservation::CALLSITE_HAS_PROFILE_WEIGHTS, - compiler->fgHaveSufficientProfileWeights()); + compiler->impInlineRoot()->fgHaveSufficientProfileWeights()); inlineResult->NoteBool(InlineObservation::CALLSITE_INSIDE_THROW_BLOCK, compiler->compCurBB->KindIs(BBJ_THROW)); bool const forceInline = (pParam->methAttr & CORINFO_FLG_FORCEINLINE) != 0; diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index d852ce9d8c0bda..4e046a6876ca8f 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1372,13 +1372,19 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value) // TODO: Enable for PgoSource::Static as well if it's not the generic profile we bundle. if (m_HasProfileWeights && (m_RootCompiler->fgHaveTrustedProfileWeights())) { + JITDUMP("Callee has trusted profile\n"); maxCodeSize = static_cast(JitConfig.JitExtDefaultPolicyMaxILProf()); } + else + { + JITDUMP("Callee has %s profile\n", m_HasProfileWeights ? "untrusted" : "no"); + } unsigned alwaysInlineSize = InlineStrategy::ALWAYS_INLINE_SIZE; if (m_InsideThrowBlock) { // Inline only small code in BBJ_THROW blocks, e.g. <= 8 bytes of IL + JITDUMP("Call site in throw block\n"); alwaysInlineSize /= 2; maxCodeSize = min(alwaysInlineSize + 1, maxCodeSize); } @@ -1401,6 +1407,7 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value) else { // Callee too big, not a candidate + JITDUMP("Callee IL size %u exceeds maxCodeSize %u\n", m_CodeSize, maxCodeSize); SetNever(InlineObservation::CALLEE_TOO_MUCH_IL); } break; @@ -1425,6 +1432,7 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value) if ((unsigned)value > bbLimit) { + JITDUMP("Callee BB count %u exceeds bbLimit %u\n", value, bbLimit); SetNever(InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS); } } From 3fc63f9249fd4c3b3a18bdd5ea39a6a95976ee00 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 16 May 2025 08:02:05 -0700 Subject: [PATCH 2/2] try boosting some but not as much --- src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/inlinepolicy.cpp | 7 ++++++- src/coreclr/jit/jitconfigvalues.h | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index eea3109e8879e9..d39bdd0350a283 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -9303,7 +9303,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, // Profile data allows us to avoid early "too many IL bytes" outs. // inlineResult->NoteBool(InlineObservation::CALLSITE_HAS_PROFILE_WEIGHTS, - compiler->impInlineRoot()->fgHaveSufficientProfileWeights()); + compiler->fgHaveSufficientProfileWeights()); inlineResult->NoteBool(InlineObservation::CALLSITE_INSIDE_THROW_BLOCK, compiler->compCurBB->KindIs(BBJ_THROW)); bool const forceInline = (pParam->methAttr & CORINFO_FLG_FORCEINLINE) != 0; diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 4e046a6876ca8f..6ddd8c6278fe02 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1372,9 +1372,14 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value) // TODO: Enable for PgoSource::Static as well if it's not the generic profile we bundle. if (m_HasProfileWeights && (m_RootCompiler->fgHaveTrustedProfileWeights())) { - JITDUMP("Callee has trusted profile\n"); + JITDUMP("Callee and root has trusted profile\n"); maxCodeSize = static_cast(JitConfig.JitExtDefaultPolicyMaxILProf()); } + else if (m_RootCompiler->fgHaveSufficientProfileWeights()) + { + JITDUMP("Root has sufficient profile\n"); + maxCodeSize = static_cast(JitConfig.JitExtDefaultPolicyMaxILRoot()); + } else { JITDUMP("Callee has %s profile\n", m_HasProfileWeights ? "untrusted" : "no"); diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 92a1cb5bf8f9ae..1bcde8ca322639 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -658,6 +658,7 @@ CONFIG_STRING(JitInlineReplayFile, "JitInlineReplayFile") // relies on PGO if it exists and generally is more aggressive. RELEASE_CONFIG_INTEGER(JitExtDefaultPolicy, "JitExtDefaultPolicy", 1) RELEASE_CONFIG_INTEGER(JitExtDefaultPolicyMaxIL, "JitExtDefaultPolicyMaxIL", 0x80) +RELEASE_CONFIG_INTEGER(JitExtDefaultPolicyMaxILRoot, "JitExtDefaultPolicyMaxILRoot", 0x100) RELEASE_CONFIG_INTEGER(JitExtDefaultPolicyMaxILProf, "JitExtDefaultPolicyMaxILProf", 0x400) RELEASE_CONFIG_INTEGER(JitExtDefaultPolicyMaxBB, "JitExtDefaultPolicyMaxBB", 7)