From f705ca3470850e042179b5237d3382ccea90d0fd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 27 Apr 2021 12:57:20 -0700 Subject: [PATCH] JIT: use profile data even when schema doesn't match After looking at some of the cases of pgo schema mismatch (#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. --- src/coreclr/jit/fgprofile.cpp | 88 +++++++++++++++++++++++++++---- src/coreclr/jit/jitconfigvalues.h | 10 +++- 2 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 85361a618a9ff..d0d1db505cca0 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -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) @@ -2172,7 +2229,7 @@ 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); } @@ -2180,7 +2237,7 @@ void EfficientEdgeCountReconstructor::Prepare() 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); } @@ -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; @@ -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" diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index d33f172b3addf..b79543bfa7ff8 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -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