From bcf72043ff8e2a970fd42032e64ff778d19afcf7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 12 Mar 2021 19:50:23 -0800 Subject: [PATCH 1/2] JIT: tolerate edge profile inconsistencies better. We should not blow up jitting just because edge counts are inconsistent. Also, make sure we're not doing any guarded devirt if we disable pgo. And, add an option JitEnablePgoRange to selectively enable use of PGO for problem isolation. --- src/coreclr/jit/compiler.cpp | 29 +++++- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/fgbasic.cpp | 1 + src/coreclr/jit/fgprofile.cpp | 152 +++++++++++++++++------------- src/coreclr/jit/importer.cpp | 10 +- src/coreclr/jit/jitconfigvalues.h | 5 +- 6 files changed, 122 insertions(+), 76 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index d3de1b50116c5..e82684aa11fc1 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -2853,17 +2853,38 @@ void Compiler::compInitOptions(JitFlags* jitFlags) fgPgoData = nullptr; fgPgoSchema = nullptr; } - // Optionally, discard the profile data. + // Optionally, disable use of profile data. // - else if (JitConfig.JitDisablePGO() != 0) + else if (JitConfig.JitDisablePgo() > 0) { - fgPgoFailReason = "PGO data available, but JitDisablePGO != 0"; + fgPgoFailReason = "PGO data available, but JitDisablePgo > 0"; fgPgoQueryResult = E_FAIL; fgPgoData = nullptr; fgPgoSchema = nullptr; + fgPgoDisabled = true; } - #ifdef DEBUG + // Optionally, enable use of profile data for only some methods. + // + else + { + static ConfigMethodRange JitEnablePgoRange; + JitEnablePgoRange.EnsureInit(JitConfig.JitEnablePgoRange()); + + // Base this decision on the root method hash, so a method either sees all available + // profile data (including that for inlinees), or none of it. + // + const unsigned hash = impInlineRoot()->info.compMethodHash(); + if (!JitEnablePgoRange.Contains(hash)) + { + fgPgoFailReason = "PGO data available, but method hash NOT within JitEnablePgoRange"; + fgPgoQueryResult = E_FAIL; + fgPgoData = nullptr; + fgPgoSchema = nullptr; + fgPgoDisabled = true; + } + } + // A successful result implies a non-NULL fgPgoSchema // if (SUCCEEDED(fgPgoQueryResult)) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 78c592a817139..d6ccbe5521918 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5640,6 +5640,7 @@ class Compiler public: const char* fgPgoFailReason; + bool fgPgoDisabled; ICorJitInfo::PgoInstrumentationSchema* fgPgoSchema; BYTE* fgPgoData; UINT32 fgPgoSchemaCount; diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 8f07bfb324673..c404cd4f5684b 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -165,6 +165,7 @@ void Compiler::fgInit() #endif fgHasSwitch = false; + fgPgoDisabled = false; fgPgoSchema = nullptr; fgPgoData = nullptr; fgPgoSchemaCount = 0; diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 8c158ddc458a1..d5fd559dce215 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -2521,6 +2521,11 @@ void Compiler::fgIncorporateEdgeCounts() // slop - profile slush fund // wbUsedSlop [out] - true if we tapped into the slush fund // +// Returns: +// true if the edge weight was adjusted +// false if the edge weight update was inconsisten with the +// edge's current [min,max} +// bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, BasicBlock* bDst, BasicBlock::weight_t slop, @@ -2556,10 +2561,8 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, } } } - else + else if (flEdgeWeightMin > newWeight) { - assert(flEdgeWeightMin > newWeight); - // We have already determined that this edge's weight // is more than newWeight, so we just allow for the slop if ((newWeight + slop) >= flEdgeWeightMin) @@ -2581,31 +2584,28 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, // If we are returning true then we should have adjusted the range so that // the newWeight is in new range [Min..Max] or fgEdgeWeightMax is zero. // Also we should have set wbUsedSlop to true. - if (result == true) + if (result) { assert((flEdgeWeightMax == BB_ZERO_WEIGHT) || ((newWeight <= flEdgeWeightMax) && (newWeight >= flEdgeWeightMin))); - if (wbUsedSlop != nullptr) - { - assert(*wbUsedSlop == true); - } + assert((wbUsedSlop == nullptr) || (*wbUsedSlop)); } } #if DEBUG - if (result == false) + if (result) + { + JITDUMP("Updated min weight of " FMT_BB " -> " FMT_BB " to [" FMT_WT ".." FMT_WT "]\n", getBlock()->bbNum, + bDst->bbNum, flEdgeWeightMin, flEdgeWeightMax); + } + else { JITDUMP("Not adjusting min weight of " FMT_BB " -> " FMT_BB "; new value " FMT_WT " not in range [" FMT_WT ".." FMT_WT "] (+/- " FMT_WT ")\n", getBlock()->bbNum, bDst->bbNum, newWeight, flEdgeWeightMin, flEdgeWeightMax, slop); result = false; // break here } - else - { - JITDUMP("Updated min weight of " FMT_BB " -> " FMT_BB " to [" FMT_WT ".." FMT_WT "]\n", getBlock()->bbNum, - bDst->bbNum, flEdgeWeightMin, flEdgeWeightMax); - } #endif // DEBUG return result; @@ -2620,6 +2620,11 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, // slop - profile slush fund // wbUsedSlop [out] - true if we tapped into the slush fund // +// Returns: +// true if the edge weight was adjusted +// false if the edge weight update was inconsisten with the +// edge's current [min,max} +// bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight, BasicBlock* bDst, BasicBlock::weight_t slop, @@ -2654,10 +2659,8 @@ bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight, } } } - else + else if (flEdgeWeightMin > newWeight) { - assert(flEdgeWeightMin > newWeight); - // We have already determined that this edge's weight // is more than newWeight, so we just allow for the slop if ((newWeight + slop) >= flEdgeWeightMin) @@ -2680,28 +2683,28 @@ bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight, // If we are returning true then we should have adjusted the range so that // the newWeight is in new range [Min..Max] or fgEdgeWeightMax is zero // Also we should have set wbUsedSlop to true, unless it is NULL - if (result == true) + if (result) { assert((flEdgeWeightMax == BB_ZERO_WEIGHT) || ((newWeight <= flEdgeWeightMax) && (newWeight >= flEdgeWeightMin))); - assert((wbUsedSlop == nullptr) || (*wbUsedSlop == true)); + assert((wbUsedSlop == nullptr) || (*wbUsedSlop)); } } #if DEBUG - if (result == false) + if (result) + { + JITDUMP("Updated max weight of " FMT_BB " -> " FMT_BB " to [" FMT_WT ".." FMT_WT "]\n", getBlock()->bbNum, + bDst->bbNum, flEdgeWeightMin, flEdgeWeightMax); + } + else { JITDUMP("Not adjusting max weight of " FMT_BB " -> " FMT_BB "; new value " FMT_WT " not in range [" FMT_WT ".." FMT_WT "] (+/- " FMT_WT ")\n", getBlock()->bbNum, bDst->bbNum, newWeight, flEdgeWeightMin, flEdgeWeightMax, slop); result = false; // break here } - else - { - JITDUMP("Updated max weight of " FMT_BB " -> " FMT_BB " to [" FMT_WT ".." FMT_WT "]\n", getBlock()->bbNum, - bDst->bbNum, flEdgeWeightMin, flEdgeWeightMax); - } #endif // DEBUG return result; @@ -3099,31 +3102,40 @@ void Compiler::fgComputeEdgeWeights() } otherEdge = fgGetPredForBlock(otherDst, bSrc); - noway_assert(edge->edgeWeightMin() <= edge->edgeWeightMax()); - noway_assert(otherEdge->edgeWeightMin() <= otherEdge->edgeWeightMax()); + // If we see min/max violations, just give up on the computations + // + const bool edgeWeightSensible = edge->edgeWeightMin() <= edge->edgeWeightMax(); + const bool otherEdgeWeightSensible = otherEdge->edgeWeightMin() <= otherEdge->edgeWeightMax(); - // Adjust edge->flEdgeWeightMin up or adjust otherEdge->flEdgeWeightMax down - diff = bSrc->bbWeight - (edge->edgeWeightMin() + otherEdge->edgeWeightMax()); - if (diff > 0) - { - assignOK &= edge->setEdgeWeightMinChecked(edge->edgeWeightMin() + diff, bDst, slop, &usedSlop); - } - else if (diff < 0) - { - assignOK &= otherEdge->setEdgeWeightMaxChecked(otherEdge->edgeWeightMax() + diff, otherDst, - slop, &usedSlop); - } + assignOK &= edgeWeightSensible && otherEdgeWeightSensible; - // Adjust otherEdge->flEdgeWeightMin up or adjust edge->flEdgeWeightMax down - diff = bSrc->bbWeight - (otherEdge->edgeWeightMin() + edge->edgeWeightMax()); - if (diff > 0) - { - assignOK &= otherEdge->setEdgeWeightMinChecked(otherEdge->edgeWeightMin() + diff, otherDst, - slop, &usedSlop); - } - else if (diff < 0) + if (assignOK) { - assignOK &= edge->setEdgeWeightMaxChecked(edge->edgeWeightMax() + diff, bDst, slop, &usedSlop); + // Adjust edge->flEdgeWeightMin up or adjust otherEdge->flEdgeWeightMax down + diff = bSrc->bbWeight - (edge->edgeWeightMin() + otherEdge->edgeWeightMax()); + if (diff > 0) + { + assignOK &= + edge->setEdgeWeightMinChecked(edge->edgeWeightMin() + diff, bDst, slop, &usedSlop); + } + else if (diff < 0) + { + assignOK &= otherEdge->setEdgeWeightMaxChecked(otherEdge->edgeWeightMax() + diff, otherDst, + slop, &usedSlop); + } + + // Adjust otherEdge->flEdgeWeightMin up or adjust edge->flEdgeWeightMax down + diff = bSrc->bbWeight - (otherEdge->edgeWeightMin() + edge->edgeWeightMax()); + if (diff > 0) + { + assignOK &= otherEdge->setEdgeWeightMinChecked(otherEdge->edgeWeightMin() + diff, otherDst, + slop, &usedSlop); + } + else if (diff < 0) + { + assignOK &= + edge->setEdgeWeightMaxChecked(edge->edgeWeightMax() + diff, bDst, slop, &usedSlop); + } } if (!assignOK) @@ -3194,33 +3206,41 @@ void Compiler::fgComputeEdgeWeights() // otherMaxEdgesWeightSum is the sum of all of the other edges flEdgeWeightMax values // This can be used to compute a lower bound for our minimum edge weight - noway_assert(maxEdgeWeightSum >= edge->edgeWeightMax()); - BasicBlock::weight_t otherMaxEdgesWeightSum = maxEdgeWeightSum - edge->edgeWeightMax(); - - // otherMinEdgesWeightSum is the sum of all of the other edges flEdgeWeightMin values - // This can be used to compute an upper bound for our maximum edge weight - noway_assert(minEdgeWeightSum >= edge->edgeWeightMin()); - BasicBlock::weight_t otherMinEdgesWeightSum = minEdgeWeightSum - edge->edgeWeightMin(); + // + BasicBlock::weight_t const otherMaxEdgesWeightSum = maxEdgeWeightSum - edge->edgeWeightMax(); - if (bDstWeight >= otherMaxEdgesWeightSum) + if (otherMaxEdgesWeightSum >= BB_ZERO_WEIGHT) { - // minWeightCalc is our minWeight when every other path to bDst takes it's flEdgeWeightMax value - BasicBlock::weight_t minWeightCalc = - (BasicBlock::weight_t)(bDstWeight - otherMaxEdgesWeightSum); - if (minWeightCalc > edge->edgeWeightMin()) + if (bDstWeight >= otherMaxEdgesWeightSum) { - assignOK &= edge->setEdgeWeightMinChecked(minWeightCalc, bDst, slop, &usedSlop); + // minWeightCalc is our minWeight when every other path to bDst takes it's flEdgeWeightMax + // value + BasicBlock::weight_t minWeightCalc = + (BasicBlock::weight_t)(bDstWeight - otherMaxEdgesWeightSum); + if (minWeightCalc > edge->edgeWeightMin()) + { + assignOK &= edge->setEdgeWeightMinChecked(minWeightCalc, bDst, slop, &usedSlop); + } } } - if (bDstWeight >= otherMinEdgesWeightSum) + // otherMinEdgesWeightSum is the sum of all of the other edges flEdgeWeightMin values + // This can be used to compute an upper bound for our maximum edge weight + // + BasicBlock::weight_t const otherMinEdgesWeightSum = minEdgeWeightSum - edge->edgeWeightMin(); + + if (otherMinEdgesWeightSum >= BB_ZERO_WEIGHT) { - // maxWeightCalc is our maxWeight when every other path to bDst takes it's flEdgeWeightMin value - BasicBlock::weight_t maxWeightCalc = - (BasicBlock::weight_t)(bDstWeight - otherMinEdgesWeightSum); - if (maxWeightCalc < edge->edgeWeightMax()) + if (bDstWeight >= otherMinEdgesWeightSum) { - assignOK &= edge->setEdgeWeightMaxChecked(maxWeightCalc, bDst, slop, &usedSlop); + // maxWeightCalc is our maxWeight when every other path to bDst takes it's flEdgeWeightMin + // value + BasicBlock::weight_t maxWeightCalc = + (BasicBlock::weight_t)(bDstWeight - otherMinEdgesWeightSum); + if (maxWeightCalc < edge->edgeWeightMax()) + { + assignOK &= edge->setEdgeWeightMaxChecked(maxWeightCalc, bDst, slop, &usedSlop); + } } } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 4299545c291b0..ef39fb4d55ac4 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21503,12 +21503,12 @@ void Compiler::considerGuardedDevirtualization( JITDUMP("Considering guarded devirtualization\n"); - // We currently only get likely class guesses when there is PGO data. So if we've disabled - // PGO, just bail out. - - if (JitConfig.JitDisablePGO() != 0) + // We currently only get likely class guesses when there is PGO data + // with class profiles. + // + if (fgPgoClassProfiles == 0) { - JITDUMP("Not guessing for class; pgo disabled\n"); + JITDUMP("Not guessing for class: no class profile pgo data, or pgo disabled\n"); return; } diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index d283522b2cdd4..055750272e335 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -463,7 +463,10 @@ CONFIG_INTEGER(JitClassProfiling, W("JitClassProfiling"), 1) CONFIG_INTEGER(JitEdgeProfiling, W("JitEdgeProfiling"), 1) // Profile consumption options -CONFIG_INTEGER(JitDisablePGO, W("JitDisablePGO"), 0) // Ignore pgo data +CONFIG_INTEGER(JitDisablePgo, W("JitDisablePgo"), 0) // Ignore pgo data for all methods +#if defined(DEBUG) +CONFIG_STRING(JitEnablePgoRange, W("JitEnablePgoRange")) // Enable pgo data for only some methods +#endif // debug // Control when Virtual Calls are expanded CONFIG_INTEGER(JitExpandCallsEarly, W("JitExpandCallsEarly"), 1) // Expand Call targets early (in the global morph From f198e5df2b683e01e3dfb0097a579939d742b95c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 28 Mar 2021 17:36:09 -0700 Subject: [PATCH 2/2] fix typos in comments --- src/coreclr/jit/fgprofile.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index d5fd559dce215..384c42d80f140 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -2523,7 +2523,7 @@ void Compiler::fgIncorporateEdgeCounts() // // Returns: // true if the edge weight was adjusted -// false if the edge weight update was inconsisten with the +// false if the edge weight update was inconsistent with the // edge's current [min,max} // bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, @@ -2622,7 +2622,7 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, // // Returns: // true if the edge weight was adjusted -// false if the edge weight update was inconsisten with the +// false if the edge weight update was inconsistent with the // edge's current [min,max} // bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight,