Skip to content

Commit

Permalink
JIT: tolerate edge profile inconsistencies better. (#50213)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Mar 29, 2021
1 parent ff465c7 commit a55b7e0
Show file tree
Hide file tree
Showing 6 changed files with 122 additions and 76 deletions.
29 changes: 25 additions & 4 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2879,17 +2879,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))
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5644,6 +5644,7 @@ class Compiler

public:
const char* fgPgoFailReason;
bool fgPgoDisabled;
ICorJitInfo::PgoInstrumentationSchema* fgPgoSchema;
BYTE* fgPgoData;
UINT32 fgPgoSchemaCount;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ void Compiler::fgInit()
#endif

fgHasSwitch = false;
fgPgoDisabled = false;
fgPgoSchema = nullptr;
fgPgoData = nullptr;
fgPgoSchemaCount = 0;
Expand Down
152 changes: 86 additions & 66 deletions src/coreclr/jit/fgprofile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 inconsistent with the
// edge's current [min,max}
//
bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight,
BasicBlock* bDst,
BasicBlock::weight_t slop,
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand All @@ -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 inconsistent with the
// edge's current [min,max}
//
bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight,
BasicBlock* bDst,
BasicBlock::weight_t slop,
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
}
}

Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/jit/importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21539,12 +21539,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;
}

Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/jit/jitconfigvalues.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a55b7e0

Please sign in to comment.