Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JIT: tolerate edge profile inconsistencies better. #50213

Merged
merged 2 commits into from
Mar 29, 2021
Merged

JIT: tolerate edge profile inconsistencies better. #50213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcf7204
JIT: tolerate edge profile inconsistencies better.
AndyAyersMS Mar 13, 2021
f198e5d
fix typos in comments
AndyAyersMS Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5640,6 +5640,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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/jit/jitconfigvalues.h
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be easier to understand in reverse? E.g., JitEnablePgo=0? Like we have COMPlus_TieredCompilation=0.

Doesn't matter to me, but sometimes negatives get confusing. We certainly aren't consistent here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since PGO is on by default and this option is mainly to do top-level triage/workaround for PGO issues, I think "disable" is less confusing.

Though when I needed to binary search I found having an "enable" less confusing.... so am not self-consistent here either.

#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