From 5b41e82f5871fe4769310fd2b616f8e100ddeb89 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 4 Feb 2021 17:53:41 -0800 Subject: [PATCH] JIT: fix interaction of PGO and jitstress (#47876) We always need to run the profile data phase so that jit stress can inject random profile counts if it so chooses. Also, clean up a few dumping nits -- don't dump the profile query status until we get around to trying to incorporate counts; summarize schema records before asserting that we must have block counts, etc. Closes #47839 --- src/coreclr/jit/compiler.cpp | 24 ++++++++------------- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/fgbasic.cpp | 4 ++++ src/coreclr/jit/fgprofile.cpp | 40 +++++++++++++++++++++++++++++------ 4 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 08fc10a85989..fbc248208598 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -2879,16 +2879,12 @@ void Compiler::compInitOptions(JitFlags* jitFlags) fgPgoSchema = nullptr; fgPgoData = nullptr; fgPgoSchemaCount = 0; + fgPgoQueryResult = E_FAIL; fgProfileData_ILSizeMismatch = false; if (jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT)) { - HRESULT hr; - hr = info.compCompHnd->getPgoInstrumentationResults(info.compMethodHnd, &fgPgoSchema, &fgPgoSchemaCount, - &fgPgoData); - - JITDUMP( - "BBOPT set; query for profile data returned hr %0x, schema at %p, counts at %p, schema element count %d\n", - hr, dspPtr(fgPgoSchema), dspPtr(fgPgoData), fgPgoSchemaCount); + fgPgoQueryResult = info.compCompHnd->getPgoInstrumentationResults(info.compMethodHnd, &fgPgoSchema, + &fgPgoSchemaCount, &fgPgoData); // a failed result that also has a non-NULL fgPgoSchema // indicates that the ILSize for the method no longer matches @@ -2896,7 +2892,7 @@ void Compiler::compInitOptions(JitFlags* jitFlags) // // We will discard the IBC data in this case // - if (FAILED(hr) && (fgPgoSchema != nullptr)) + if (FAILED(fgPgoQueryResult) && (fgPgoSchema != nullptr)) { fgProfileData_ILSizeMismatch = true; fgPgoData = nullptr; @@ -2905,7 +2901,7 @@ void Compiler::compInitOptions(JitFlags* jitFlags) #ifdef DEBUG // A successful result implies a non-NULL fgPgoSchema // - if (SUCCEEDED(hr)) + if (SUCCEEDED(fgPgoQueryResult)) { assert(fgPgoSchema != nullptr); } @@ -2913,7 +2909,7 @@ void Compiler::compInitOptions(JitFlags* jitFlags) // A failed result implies a NULL fgPgoSchema // see implementation of Compiler::fgHaveProfileData() // - if (FAILED(hr)) + if (FAILED(fgPgoQueryResult)) { assert(fgPgoSchema == nullptr); } @@ -4400,14 +4396,12 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags compFunctionTraceStart(); - // If profile data is available, incorporate it into the flowgraph. + // Incorporate profile data. + // // Note: the importer is sensitive to block weights, so this has // to happen before importation. // - if (compileFlags->IsSet(JitFlags::JIT_FLAG_BBOPT) && fgHaveProfileData()) - { - DoPhase(this, PHASE_INCPROFILE, &Compiler::fgIncorporateProfileData); - } + DoPhase(this, PHASE_INCPROFILE, &Compiler::fgIncorporateProfileData); // Import: convert the instrs in each basic block to a tree based intermediate representation // diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b6f23a480b3c..78c8a31fe2de 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5539,6 +5539,7 @@ class Compiler ICorJitInfo::PgoInstrumentationSchema* fgPgoSchema; BYTE* fgPgoData; UINT32 fgPgoSchemaCount; + HRESULT fgPgoQueryResult; UINT32 fgNumProfileRuns; UINT32 fgPgoBlockCounts; UINT32 fgPgoClassProfiles; diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 1d809cbee240..37c06e082f85 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -2251,6 +2251,10 @@ void Compiler::fgFindBasicBlocks() { printf("*************** In fgFindBasicBlocks() for %s\n", info.compFullName); } + + // Call this here so any dump printing it inspires doesn't appear in the bb table. + // + fgStressBBProf(); #endif // Allocate the 'jump target' bit vector diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 3d768985bdf2..d0a6080a615e 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -885,11 +885,38 @@ PhaseStatus Compiler::fgInstrumentMethod() // PhaseStatus Compiler::fgIncorporateProfileData() { - assert(fgHaveProfileData()); + // Are we doing profile stress? + // + if (fgStressBBProf() > 0) + { + JITDUMP("JitStress -- incorporating random profile data\n"); + fgIncorporateBlockCounts(); + return PhaseStatus::MODIFIED_EVERYTHING; + } + + // Do we have profile data? + // + if (!fgHaveProfileData()) + { + if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT)) + { + JITDUMP("BBOPT set, but no profile data available (hr=%08x)\n", fgPgoQueryResult); + } + else + { + JITDUMP("BBOPT not set\n"); + } + return PhaseStatus::MODIFIED_NOTHING; + } // Summarize profile data // - fgNumProfileRuns = 0; + JITDUMP("Have profile data: %d schema records (schema at %p, data at %p)\n", fgPgoSchemaCount, dspPtr(fgPgoSchema), + dspPtr(fgPgoData)); + + fgNumProfileRuns = 0; + unsigned otherRecords = 0; + for (UINT32 iSchema = 0; iSchema < fgPgoSchemaCount; iSchema++) { switch (fgPgoSchema[iSchema].InstrumentationKind) @@ -907,19 +934,20 @@ PhaseStatus Compiler::fgIncorporateProfileData() break; default: + otherRecords++; break; } } - assert(fgPgoBlockCounts > 0); - if (fgNumProfileRuns == 0) { fgNumProfileRuns = 1; } - JITDUMP("Profile summary: %d runs, %d block probes, %d class profiles\n", fgNumProfileRuns, fgPgoBlockCounts, - fgPgoClassProfiles); + JITDUMP("Profile summary: %d runs, %d block probes, %d class profiles, %d other records\n", fgNumProfileRuns, + fgPgoBlockCounts, fgPgoClassProfiles, otherRecords); + + assert(fgPgoBlockCounts > 0); fgIncorporateBlockCounts(); return PhaseStatus::MODIFIED_EVERYTHING;