From 8f8a5c17bf1a41c613fb136b33f600e7559ce2ca Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 30 Apr 2024 14:04:56 -0700 Subject: [PATCH 1/5] JIT: synthesize PGO if no schema, when dynamic PGO is active If we know dynamic PGO is active, and we do not find a PGO schema for a method, synthesize PGO data. The schema may be missing if the method was prejitted but not covered by static PGO, or was considered too simple to need profiling (aka minimal profiling). This synthesis removes the possibility of a mixed PGO/no PGO situation. These are problematic, especially in methods that do a lot of inlining. Now when dynamic PGO is active all methods that get optimized will have some form of PGO data. Contributes to #93020. --- src/coreclr/jit/fgprofile.cpp | 36 +++++++++++++++++------------------ src/coreclr/jit/optimizer.cpp | 2 +- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 10fdd6035e0b3..325632fd18218 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -18,26 +18,15 @@ // true if so // // Note: -// This now returns true for inlinees. We might consider preserving the -// old behavior for crossgen, since crossgen BBINSTRs still do inlining -// and don't instrument the inlinees. +// In most cases it is more appropriate to call fgHaveProfileWeights, +// since that tells you if blocks have profile-based weights. // -// Thus if BBINSTR and BBOPT do the same inlines (which can happen) -// profile data for an inlinee (if available) will not fully reflect -// the behavior of the inlinee when called from this method. +// This method literally checks if the runtime had a profile schema, +// from which we can derive weights. // -// If this inlinee was not inlined by the BBINSTR run then the -// profile data for the inlinee will reflect this method's influence. -// -// * for ALWAYS_INLINE and FORCE_INLINE cases it is unlikely we'll find -// any profile data, as BBINSTR and BBOPT callers will both inline; -// only indirect callers will invoke the instrumented version to run. -// * for DISCRETIONARY_INLINE cases we may or may not find relevant -// data, depending, but chances are the data is relevant. -// -// TieredPGO data comes from Tier0 methods, which currently do not do -// any inlining; thus inlinee profile data should be available and -// representative. +// Schema-based data comes from Tier0 methods, which currently do not do +// any inlining; thus inlinee profile data should be available and +// representative. // bool Compiler::fgHaveProfileData() { @@ -47,6 +36,9 @@ bool Compiler::fgHaveProfileData() //------------------------------------------------------------------------ // fgHaveProfileWeights: Check if we have a profile that has weights. // +// Notes: +// These weights may come from instrumentation or from synthesis. +// bool Compiler::fgHaveProfileWeights() { return fgPgoHaveWeights; @@ -2887,6 +2879,14 @@ PhaseStatus Compiler::fgIncorporateProfileData() JITDUMP("BBOPT not set\n"); } + // Is dynamic PGO active? If so, run synthesis. + // + if (fgPgoDynamic) + { + JITDUMP("Dynamic PGO active, synthesizing profile data\n"); + ProfileSynthesis::Run(this, ProfileSynthesisOption::AssignLikelihoods); + } + // Scale the "synthetic" block weights. // fgApplyProfileScale(); diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 0de4cacc7bb0c..cfd61a6af6aa0 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -234,7 +234,7 @@ void Compiler::optScaleLoopBlocks(BasicBlock* begBlk, BasicBlock* endBlk) for (BasicBlock* const curBlk : BasicBlockRangeList(begBlk, endBlk)) { // Don't change the block weight if it came from profile data. - if (curBlk->hasProfileWeight() && fgHaveProfileData()) + if (curBlk->hasProfileWeight() && fgHaveProfileWeights()) { reportBlockWeight(curBlk, "; unchanged: has profile weight"); continue; From 828e4c22fe0f10b0038d0786f2bfbd21d70f60f0 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 1 May 2024 12:57:19 -0700 Subject: [PATCH 2/5] don't run profile incorporation if not optimizing --- src/coreclr/jit/fgprofile.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 325632fd18218..c72aef70b9a2e 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -2829,6 +2829,14 @@ PhaseStatus Compiler::fgInstrumentMethod() // PhaseStatus Compiler::fgIncorporateProfileData() { + // For now we only rely on profile data when optimizing. + // + if (!opts.OptimizationEnabled()) + { + JITDUMP("not optimizing, so not incorproating any profile data\n"); + return PhaseStatus::MODIFIED_NOTHING; + } + // Are we doing profile stress? // if (fgStressBBProf() > 0) From 4a8729a676d4698764909025051590d32bbd33bf Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 1 May 2024 17:34:01 -0700 Subject: [PATCH 3/5] fix typo --- src/coreclr/jit/fgprofile.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index c72aef70b9a2e..669e2b93270c9 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -2833,7 +2833,7 @@ PhaseStatus Compiler::fgIncorporateProfileData() // if (!opts.OptimizationEnabled()) { - JITDUMP("not optimizing, so not incorproating any profile data\n"); + JITDUMP("not optimizing, so not incorporating any profile data\n"); return PhaseStatus::MODIFIED_NOTHING; } From c77902680031c2f21613143144012f72e700b2a4 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 1 May 2024 23:19:59 -0700 Subject: [PATCH 4/5] reset BBOPT on failover; allow profile stress even if not optimizing --- src/coreclr/jit/compiler.cpp | 1 + src/coreclr/jit/fgprofile.cpp | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index f15820a7dc574..9e2e3525ad75a 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -8056,6 +8056,7 @@ if (!inlineInfo && compileFlags->Set(JitFlags::JIT_FLAG_MIN_OPT); compileFlags->Clear(JitFlags::JIT_FLAG_SIZE_OPT); compileFlags->Clear(JitFlags::JIT_FLAG_SPEED_OPT); + compileFlags->Clear(JitFlags::JIT_FLAG_BBOPT); goto START; } diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 669e2b93270c9..6d7ef484fe9a8 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -2829,14 +2829,6 @@ PhaseStatus Compiler::fgInstrumentMethod() // PhaseStatus Compiler::fgIncorporateProfileData() { - // For now we only rely on profile data when optimizing. - // - if (!opts.OptimizationEnabled()) - { - JITDUMP("not optimizing, so not incorporating any profile data\n"); - return PhaseStatus::MODIFIED_NOTHING; - } - // Are we doing profile stress? // if (fgStressBBProf() > 0) @@ -2848,6 +2840,14 @@ PhaseStatus Compiler::fgIncorporateProfileData() return PhaseStatus::MODIFIED_EVERYTHING; } + // For now we only rely on profile data when optimizing. + // + if (!opts.OptimizationEnabled()) + { + JITDUMP("not optimizing, so not incorporating any profile data\n"); + return PhaseStatus::MODIFIED_NOTHING; + } + #ifdef DEBUG // Optionally run synthesis // From 844d9ae0b763caea90b218efbc0d9d5ec34fbfd4 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 2 May 2024 09:57:59 -0700 Subject: [PATCH 5/5] reset pgo vars when switching to minopts after reading PGO data --- src/coreclr/jit/compiler.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 9e2e3525ad75a..8e82707adbbbf 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4088,7 +4088,18 @@ void Compiler::compSetOptimizationLevel() { info.compCompHnd->setMethodAttribs(info.compMethodHnd, CORINFO_FLG_SWITCHED_TO_MIN_OPT); opts.jitFlags->Clear(JitFlags::JIT_FLAG_TIER1); + opts.jitFlags->Clear(JitFlags::JIT_FLAG_BBOPT); compSwitchedToMinOpts = true; + + // We may have read PGO data. Clear it out because we won't be using it. + // + fgPgoFailReason = "method switched to min-opts"; + fgPgoQueryResult = E_FAIL; + fgPgoHaveWeights = false; + fgPgoData = nullptr; + fgPgoSchema = nullptr; + fgPgoDisabled = true; + fgPgoDynamic = false; } #ifdef DEBUG