Skip to content

Commit

Permalink
JIT: use profile data even when schema doesn't match
Browse files Browse the repository at this point in the history
After looking at some of the cases of pgo schema mismatch (dotnet#51908), the pgo
schemas still seem to have partial validity, so by default the jit will now
use the schema and data even for mismatches.

Add the ability to optionally assert on mismatches.

Also add a mode where the the jit compares the schema it provided to the
the runtime with the schema the runtime gives back to the jit, to try and
locate possible schema compression/decompression issues that might lead to the
sorts of mismatches we're seeing.

This turns out not to have found any issues. All the mismatches we see are
from schemas provided to the jit via static PGO. But it seems like a useful
capability to retain.
  • Loading branch information
AndyAyersMS committed Apr 28, 2021
1 parent d44bd23 commit f705ca3
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 11 deletions.
88 changes: 79 additions & 9 deletions src/coreclr/jit/fgprofile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1638,6 +1638,63 @@ PhaseStatus Compiler::fgInstrumentMethod()
return PhaseStatus::MODIFIED_NOTHING;
}

#ifdef DEBUG
if (JitConfig.JitVerifyPgoSchema() > 0)
{
JITDUMP("Verifying schema roundtrip (runtime compression/decompression)\n");

ICorJitInfo::PgoInstrumentationSchema* rtPgoSchema = nullptr;
BYTE* rtPgoData = 0;
UINT32 rtPgoSchemaCount = 0;
HRESULT rtPgoQueryResult = info.compCompHnd->getPgoInstrumentationResults(info.compMethodHnd, &rtPgoSchema,
&rtPgoSchemaCount, &rtPgoData);

assert(SUCCEEDED(rtPgoQueryResult));
assert(rtPgoSchemaCount == (UINT32)schema.size());

bool failed = false;

for (UINT32 i = 0; i < rtPgoSchemaCount; i++)
{
if (schema[i].InstrumentationKind != rtPgoSchema[i].InstrumentationKind)
{
JITDUMP("Kind differs at entry %u: %u/%u\n", i, schema[i].InstrumentationKind,
rtPgoSchema[i].InstrumentationKind);
failed = true;
}

if (schema[i].ILOffset != rtPgoSchema[i].ILOffset)
{
JITDUMP("ILOffset differs at entry %u: %u/%u\n", i, schema[i].ILOffset, rtPgoSchema[i].ILOffset);
failed = true;
}

if (schema[i].Count != rtPgoSchema[i].Count)
{
JITDUMP("Count differs at entry %u: %u/%u\n", i, schema[i].Count, rtPgoSchema[i].Count);
failed = true;
}

if (schema[i].Other != rtPgoSchema[i].Other)
{
JITDUMP("Other differs at entry %u: %u/%u\n", i, schema[i].Other, rtPgoSchema[i].Other);
failed = true;
}
}

if (JitConfig.JitVerifyPgoSchema() > 1)
{
assert(!failed);
}

JITDUMP("Schema round-trip %s\n", failed ? "failed" : "succeeded");
}
#endif

// The runtime stores the schema in a compressed format. Ask for the PGO data back and verify
// we get back the same schema we provided. THis sanity checks the compression/decompression
// done by the runtime.

// Add the instrumentation code
//
for (BasicBlock* block = fgFirstBB; (block != nullptr); block = block->bbNext)
Expand Down Expand Up @@ -2172,15 +2229,15 @@ void EfficientEdgeCountReconstructor::Prepare()

if (!m_keyToBlockMap.Lookup(schemaEntry.ILOffset, &sourceBlock))
{
JITDUMP("Could not find source block for schema entry %d (IL offset/key %08x\n", iSchema,
JITDUMP("Could not find source block for schema entry %d (IL offset/key %08x)\n", iSchema,
schemaEntry.ILOffset);
}

BasicBlock* targetBlock = nullptr;

if (!m_keyToBlockMap.Lookup(schemaEntry.Other, &targetBlock))
{
JITDUMP("Could not find target block for schema entry %d (IL offset/key %08x\n", iSchema,
JITDUMP("Could not find target block for schema entry %d (IL offset/key %08x)\n", iSchema,
schemaEntry.ILOffset);
}

Expand Down Expand Up @@ -2220,13 +2277,26 @@ void EfficientEdgeCountReconstructor::Solve()
{
// If issues arose earlier, then don't try solving.
//
if (m_badcode || m_mismatch || m_allWeightsZero)
if (m_badcode || m_allWeightsZero || (m_mismatch && (JitConfig.JitIgnoreSchemaMismatch() == 0)))
{
JITDUMP("... not solving because of the %s\n",
m_badcode ? "badcode" : m_allWeightsZero ? "zero counts" : "mismatch");
return;
}

// Note because of the sparsity of edge based profiles, we should be able to reach a solution
// even if we lose or mis-attribute some of the counters. So carrying on here despite a mismatch,
// while not ideal, is not as crazy as it might seen.
//
if (m_mismatch)
{
if (JitConfig.JitIgnoreSchemaMismatch() > 1)
{
assert(!"PGO schema mismatch");
}
JITDUMP("... attempting to solve despite mismatched schema.");
}

unsigned nPasses = 0;
unsigned const nLimit = 10;

Expand Down Expand Up @@ -2470,17 +2540,17 @@ void EfficientEdgeCountReconstructor::Solve()
//
void EfficientEdgeCountReconstructor::Propagate()
{
// We don't expect mismatches or convergence failures.
// We don't expect convergence failures.
//
// Mismatches are optionally tolerated as the flow for static pgo doesn't prevent them now,
// and we may still be able to extract some useful information.
//

// Mismatches are currently expected as the flow for static pgo doesn't prevent them now.
// assert(!m_mismatch);

assert(!m_failedToConverge);

// If any issues arose during reconstruction, don't set weights.
//
if (m_badcode || m_mismatch || m_failedToConverge || m_allWeightsZero)
if (m_badcode || m_failedToConverge || m_allWeightsZero ||
(m_mismatch && (JitConfig.JitIgnoreSchemaMismatch() == 0)))
{
JITDUMP("... discarding profile data because of %s\n",
m_badcode ? "badcode" : m_mismatch ? "mismatch" : m_allWeightsZero ? "zero counts"
Expand Down
10 changes: 8 additions & 2 deletions src/coreclr/jit/jitconfigvalues.h
Original file line number Diff line number Diff line change
Expand Up @@ -476,11 +476,17 @@ CONFIG_INTEGER(JitMinimalPrejitProfiling, W("JitMinimalPrejitProfiling"), 0)
CONFIG_INTEGER(JitClassProfiling, W("JitClassProfiling"), 1)
CONFIG_INTEGER(JitEdgeProfiling, W("JitEdgeProfiling"), 1)

#if defined(DEBUG)
CONFIG_INTEGER(JitVerifyPgoSchema, W("JitVerifyPgoSchema"), 0) // Validate runtime schema compression/decompression
#endif

// Profile consumption options
CONFIG_INTEGER(JitDisablePgo, W("JitDisablePgo"), 0) // Ignore pgo data for all methods
CONFIG_INTEGER(JitDisablePgo, W("JitDisablePgo"), 0) // Ignore pgo data for all methods
CONFIG_INTEGER(JitIgnoreSchemaMismatch, W("JitIgnoreSchemaMismatch"), 1) // Try and use pgo data even if schema doesn't
// match up
#if defined(DEBUG)
CONFIG_STRING(JitEnablePgoRange, W("JitEnablePgoRange")) // Enable pgo data for only some methods
#endif // debug
#endif

// Control when Virtual Calls are expanded
CONFIG_INTEGER(JitExpandCallsEarly, W("JitExpandCallsEarly"), 1) // Expand Call targets early (in the global morph
Expand Down

0 comments on commit f705ca3

Please sign in to comment.