From 333ed572450f6ab5199c97cc9237519a8f943e3f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 18 Apr 2025 10:50:49 -0700 Subject: [PATCH 1/2] JIT: don't mark callees noinline for non-fatal observations with pgo Some inline policy decisions are sensitive to the root method having valid PGO data. And sometimes the decision is that a particular callee can never be a viable inline. If so, the policy will have the runtime mark the method as `NoInlining` to short-circuit future inlining decisions and save some JIT throughput. But if the method had valid PGO data the policy may have made a different decision. By marking this method noinline, the JIT will immediately reject all future inline attempts (say from other calling methods with valid PGO data). For instance, without PGO, the policy may limit the number of basic blocks in a viable callee to 5. But with PGO the policy is willing to consider callees with many more blocks. In this PR we modify the reporting policy so that if TieredPGO is active, we only ever report back methods with truly fatal inlining issues, not just "informational" or "performance" issues, regardless of the state of the PGO data for the method being jitted. --- src/coreclr/jit/inline.cpp | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index b0052309a29699..62eeb91955ba1f 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -792,16 +792,39 @@ void InlineResult::Report() // IS_NOINLINE, then we've uncovered a reason why this method // can't ever be inlined. Update the callee method attributes // so that future inline attempts for this callee fail faster. - + // InlineObservation obs = m_Policy->GetObservation(); - if ((m_Callee != nullptr) && (obs != InlineObservation::CALLEE_IS_NOINLINE)) + bool report = (m_Callee != nullptr); + bool suppress = (obs == InlineObservation::CALLEE_IS_NOINLINE); + bool dynamicPgo = m_RootCompiler->fgPgoDynamic; + + // If dynamic pgo is active, only propagate noinline back to metadata + // when there is a CALLEE FATAL observation. We want to make sure + // not to block future inlines based on performance or throughput considerations. + // + // Note fgPgoDyanmic (and hence dynamicPgo) is true iff TieredPGO is enabled globally. + // In particular this value does not depend on the root method having PGO data. + // + if (dynamicPgo) + { + InlineTarget target = InlGetTarget(obs); + InlineImpact impact = InlGetImpact(obs); + suppress = (target != InlineTarget::CALLEE) || (impact != InlineImpact::FATAL); + } + + if (report && !suppress) { - JITDUMP("\nINLINER: Marking %s as NOINLINE because of %s\n", callee, InlGetObservationString(obs)); + JITDUMP("\nINLINER: Marking %s as NOINLINE (observation %s)\n", callee, InlGetObservationString(obs)); COMP_HANDLE comp = m_RootCompiler->info.compCompHnd; comp->setMethodAttribs(m_Callee, CORINFO_FLG_BAD_INLINEE); } + else if (suppress) + { + JITDUMP("\nINLINER: Not marking %s NOINLINE; %s (observation %s)\n", callee, + dynamicPgo ? "pgo active" : "already known", InlGetObservationString(obs)); + } } if (IsDecided() || m_reportFailureAsVmFailure || m_successResult != INLINE_PASS) From 47efc87bcdf7d42752cb262ab230f087c16e6bec Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 18 Apr 2025 11:11:21 -0700 Subject: [PATCH 2/2] Update src/coreclr/jit/inline.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/coreclr/jit/inline.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 62eeb91955ba1f..7ce398b57feef3 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -803,7 +803,7 @@ void InlineResult::Report() // when there is a CALLEE FATAL observation. We want to make sure // not to block future inlines based on performance or throughput considerations. // - // Note fgPgoDyanmic (and hence dynamicPgo) is true iff TieredPGO is enabled globally. + // Note fgPgoDynamic (and hence dynamicPgo) is true iff TieredPGO is enabled globally. // In particular this value does not depend on the root method having PGO data. // if (dynamicPgo)