From 70340735ddd37d24c10fecdafe0235203141f4fe Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 4 Feb 2021 13:16:42 -0800 Subject: [PATCH] JIT: fix interaction of PGO and jitstress 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 08fc10a8598970..fbc2482085988b 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 b6f23a480b3c2e..78c8a31fe2decc 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 1d809cbee2402c..37c06e082f85d1 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 3d768985bdf2d9..d0a6080a615eb2 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;