Skip to content

Commit

Permalink
JIT: fix interaction of PGO and jitstress (#47876)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
AndyAyersMS committed Feb 5, 2021
1 parent 9a283e1 commit 5b41e82
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 21 deletions.
24 changes: 9 additions & 15 deletions src/coreclr/jit/compiler.cpp
Expand Up @@ -2879,24 +2879,20 @@ 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
// the ILSize for the method when profile data was collected.
//
// We will discard the IBC data in this case
//
if (FAILED(hr) && (fgPgoSchema != nullptr))
if (FAILED(fgPgoQueryResult) && (fgPgoSchema != nullptr))
{
fgProfileData_ILSizeMismatch = true;
fgPgoData = nullptr;
Expand All @@ -2905,15 +2901,15 @@ void Compiler::compInitOptions(JitFlags* jitFlags)
#ifdef DEBUG
// A successful result implies a non-NULL fgPgoSchema
//
if (SUCCEEDED(hr))
if (SUCCEEDED(fgPgoQueryResult))
{
assert(fgPgoSchema != nullptr);
}

// A failed result implies a NULL fgPgoSchema
// see implementation of Compiler::fgHaveProfileData()
//
if (FAILED(hr))
if (FAILED(fgPgoQueryResult))
{
assert(fgPgoSchema == nullptr);
}
Expand Down Expand Up @@ -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
//
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
Expand Up @@ -5539,6 +5539,7 @@ class Compiler
ICorJitInfo::PgoInstrumentationSchema* fgPgoSchema;
BYTE* fgPgoData;
UINT32 fgPgoSchemaCount;
HRESULT fgPgoQueryResult;
UINT32 fgNumProfileRuns;
UINT32 fgPgoBlockCounts;
UINT32 fgPgoClassProfiles;
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/fgbasic.cpp
Expand Up @@ -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
Expand Down
40 changes: 34 additions & 6 deletions src/coreclr/jit/fgprofile.cpp
Expand Up @@ -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)
Expand All @@ -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;
Expand Down

0 comments on commit 5b41e82

Please sign in to comment.