Skip to content

Commit

Permalink
JIT: start checking sum of successor likelihoods (#99024)
Browse files Browse the repository at this point in the history 
Add the ability to check that successor edge likelihoods sum to 1.0
Verify that successor edge likelihoods sum to 1.0 for the phases up through
and including `fgAddInternal`.

Fixed a few problems
* importer branch folding needs to update likelihoods
* gdv likelihood needed some adjustments, especially for the multi-guess case
* profile incorporation for OSR methods also needed some fixing. Here
we may have inconsistencies so the right answer is not always clear.
With this change we now trust the successor edge counts even if they
do not sum to the block count.

Contributes to #93020.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Feb 29, 2024
1 parent 2a93c24 commit 54b4a60
Show file tree
Hide file tree
Showing 7 changed files with 146 additions and 145 deletions.
18 changes: 18 additions & 0 deletions src/coreclr/jit/block.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,24 @@ unsigned SsaStressHashHelper()
}
#endif

//------------------------------------------------------------------------
// setLikelihood: set the likelihood of aflow edge
//
// Arguments:
// likelihood -- value in range [0.0, 1.0] indicating how likely
// the source block is to transfer control along this edge.
//
void FlowEdge::setLikelihood(weight_t likelihood)
{
assert(likelihood >= 0.0);
assert(likelihood <= 1.0);
m_likelihoodSet = true;
m_likelihood = likelihood;

JITDUMP("setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT "\n", m_sourceBlock->bbNum, m_destBlock->bbNum,
m_likelihood);
}

//------------------------------------------------------------------------
// AllSuccessorEnumerator: Construct an instance of the enumerator.
//
Expand Down
8 changes: 1 addition & 7 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,13 +635,7 @@ struct FlowEdge
return m_likelihood;
}

void setLikelihood(weight_t likelihood)
{
assert(likelihood >= 0.0);
assert(likelihood <= 1.0);
m_likelihoodSet = true;
m_likelihood = likelihood;
}
void setLikelihood(weight_t likelihood);

void clearLikelihood()
{
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1584,9 +1584,10 @@ enum class ProfileChecks : unsigned int
CHECK_NONE = 0,
CHECK_CLASSIC = 1 << 0, // check "classic" jit weights
CHECK_HASLIKELIHOOD = 1 << 1, // check all FlowEdges for hasLikelihood
CHECK_LIKELY = 1 << 2, // fully check likelihood based weights
RAISE_ASSERT = 1 << 3, // assert on check failure
CHECK_ALL_BLOCKS = 1 << 4, // check blocks even if bbHasProfileWeight is false
CHECK_LIKELIHOODSUM = 1 << 2, // check block succesor likelihoods sum to 1
CHECK_LIKELY = 1 << 3, // fully check likelihood based weights
RAISE_ASSERT = 1 << 4, // assert on check failure
CHECK_ALL_BLOCKS = 1 << 5, // check blocks even if bbHasProfileWeight is false
};

inline constexpr ProfileChecks operator ~(ProfileChecks a)
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/fginline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,9 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
m_compiler->fgRemoveRefPred(block->GetFalseEdge());
block->SetKind(BBJ_ALWAYS);
}

FlowEdge* const edge = m_compiler->fgGetPredForBlock(block->GetTarget(), block);
edge->setLikelihood(1.0);
}
}
else
Expand Down
121 changes: 33 additions & 88 deletions src/coreclr/jit/fgprofile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3862,18 +3862,22 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block,
{
// We expect one pseudo-edge and at least one normal edge.
//
Edge* pseudoEdge = nullptr;
unsigned nEdges = 0;
Edge* pseudoEdge = nullptr;
weight_t pseudoEdgeWeight = 0;
unsigned nEdges = 0;
weight_t successorWeight = BB_ZERO_WEIGHT;

for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge)
{
if (edge->m_isPseudoEdge)
{
assert(pseudoEdge == nullptr);
pseudoEdge = edge;
pseudoEdge = edge;
pseudoEdgeWeight = edge->m_weight;
continue;
}

successorWeight += edge->m_weight;
nEdges++;
}

Expand All @@ -3890,28 +3894,25 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block,

assert(nEdges == nSucc);

if (info->m_weight == BB_ZERO_WEIGHT)
if ((info->m_weight == BB_ZERO_WEIGHT) || (successorWeight == BB_ZERO_WEIGHT))
{
JITDUMP("\nPropagate: OSR entry block weight is zero\n");
JITDUMP("\nPropagate: OSR entry block or successor weight is zero\n");
EntryWeightZero();
return;
}

// Transfer model edge weight onto the FlowEdges as likelihoods.
//
assert(nEdges == nSucc);
weight_t totalLikelihood = 0;
JITDUMP("Normalizing OSR successor likelihoods with factor 1/" FMT_WT "\n", successorWeight);

for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge)
{
assert(block == edge->m_sourceBlock);

// The pseudo edge doesn't correspond to a flow edge,
// but it carries away some flow.
// The pseudo edge doesn't correspond to a flow edge.
//
if (edge == pseudoEdge)
{
totalLikelihood += edge->m_weight / info->m_weight;
continue;
}

Expand All @@ -3927,51 +3928,19 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block,

if (nEdges == 1)
{
// Conceptually we could assert(edge->m_weight == info->m_weight);
// but we can have inconsistencies.
//
// Go with what we know for sure, edge should be 100% likely.
//
likelihood = 1.0;
JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (uniq)\n", block->bbNum,
edge->m_targetBlock->bbNum, likelihood);
flowEdge->setLikelihood(likelihood);
totalLikelihood += likelihood;
break;
}

assert(info->m_weight != BB_ZERO_WEIGHT);

// We may see nonsensical weights here, cap likelihood.
//
bool capped = false;
if (edge->m_weight > info->m_weight)
{
capped = true;
likelihood = 1.0;
}
else
{
likelihood = edge->m_weight / info->m_weight;
}
JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (%s)\n", block->bbNum,
edge->m_targetBlock->bbNum, likelihood, capped ? "pgo -- capped" : "pgo");
likelihood = edge->m_weight / successorWeight;
JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (pgo)\n", block->bbNum,
edge->m_targetBlock->bbNum, likelihood);
flowEdge->setLikelihood(likelihood);
totalLikelihood += likelihood;
}

// Note we expect real flow imbalances here as it's likely there
// was no observed flow from the OSR entry to some of its successors.
// Since we added in the pseudo edge likelihood above, the check below
// probably won't flag this.
//
// Seems like for OSR we will always want to run synthesis/repair.
//
if (totalLikelihood != 1.0)
{
// Consider what to do here... flag this method as needing immediate profile repairs?
//
JITDUMP(FMT_BB " total outgoing likelihood inaccurate: " FMT_WT "\n", block->bbNum, totalLikelihood);
}
}

Expand All @@ -3984,19 +3953,16 @@ void EfficientEdgeCountReconstructor::PropagateOSREntryEdges(BasicBlock* block,
// info - model info for the block
// nSucc - number of successors of the block in the flow graph
//
// Notes:
// This block requires special handling because original method flow
// was interrupted here.
//
void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInfo* info, unsigned nSucc)
{
// There is at least one FlowEdge.
//
// Check the reconstruction graph edges. For normal blocks, if we have
// any pseudo-edges there should be only one pseudo-edge, and no regular edges.
//
Edge* pseudoEdge = nullptr;
unsigned nEdges = 0;
Edge* pseudoEdge = nullptr;
unsigned nEdges = 0;
weight_t successorWeight = BB_ZERO_WEIGHT;

for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge)
{
Expand All @@ -4007,6 +3973,7 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf
continue;
}

successorWeight += edge->m_weight;
nEdges++;
}

Expand Down Expand Up @@ -4046,7 +4013,7 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf
//
// (TODO: use synthesis here)
//
if ((nEdges != nSucc) || (info->m_weight == BB_ZERO_WEIGHT))
if ((nEdges != nSucc) || (info->m_weight == BB_ZERO_WEIGHT) || (successorWeight == BB_ZERO_WEIGHT))
{
JITDUMP(FMT_BB " %s , setting outgoing likelihoods heuristically\n", block->bbNum,
(nEdges != nSucc) ? "has inaccurate flow model" : "has zero weight");
Expand All @@ -4067,7 +4034,7 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf
// Transfer model edge weight onto the FlowEdges as likelihoods.
//
assert(nEdges == nSucc);
weight_t totalLikelihood = 0;
JITDUMP("Normalizing successor likelihoods with factor 1/" FMT_WT "\n", successorWeight);

for (Edge* edge = info->m_outgoingEdges; edge != nullptr; edge = edge->m_nextOutgoingEdge)
{
Expand All @@ -4079,45 +4046,17 @@ void EfficientEdgeCountReconstructor::PropagateEdges(BasicBlock* block, BlockInf
if (nEdges == 1)
{
assert(nSucc == 1);

// Conceptually we could assert(edge->m_weight == info->m_weight);
// but we can have inconsistencies.
//
// Go with what we know for sure, edge should be 100% likely.
//
likelihood = 1.0;
JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (uniq)\n", block->bbNum,
edge->m_targetBlock->bbNum, likelihood);
flowEdge->setLikelihood(likelihood);
totalLikelihood += likelihood;
break;
}

assert(info->m_weight != BB_ZERO_WEIGHT);

// We may see nonsensical weights here, cap likelihood.
//
bool capped = false;
if (edge->m_weight > info->m_weight)
{
capped = true;
likelihood = 1.0;
}
else
{
likelihood = edge->m_weight / info->m_weight;
}
JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (%s)\n", block->bbNum,
edge->m_targetBlock->bbNum, likelihood, capped ? "pgo -- capped" : "pgo");
likelihood = edge->m_weight / successorWeight;
JITDUMP("Setting likelihood of " FMT_BB " -> " FMT_BB " to " FMT_WT " (pgo)\n", block->bbNum,
edge->m_targetBlock->bbNum, likelihood);
flowEdge->setLikelihood(likelihood);
totalLikelihood += likelihood;
}

if (totalLikelihood != 1.0)
{
// Consider what to do here... flag this method as needing immediate profile repairs?
//
JITDUMP(FMT_BB " total outgoing likelihood inaccurate: " FMT_WT "\n", block->bbNum, totalLikelihood);
}
}

Expand Down Expand Up @@ -5296,7 +5235,8 @@ void Compiler::fgDebugCheckProfileWeights()
}
else
{
ProfileChecks checks = ProfileChecks::CHECK_HASLIKELIHOOD | ProfileChecks::RAISE_ASSERT;
ProfileChecks checks =
ProfileChecks::CHECK_HASLIKELIHOOD | ProfileChecks::CHECK_LIKELIHOODSUM | ProfileChecks::RAISE_ASSERT;
fgDebugCheckProfileWeights(checks);
}
}
Expand Down Expand Up @@ -5328,6 +5268,7 @@ void Compiler::fgDebugCheckProfileWeights(ProfileChecks checks)
const bool verifyClassicWeights = fgEdgeWeightsComputed && hasFlag(checks, ProfileChecks::CHECK_CLASSIC);
const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY);
const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD);
const bool verifyLikelihoodSum = hasFlag(checks, ProfileChecks::CHECK_LIKELIHOODSUM);
const bool assertOnFailure = hasFlag(checks, ProfileChecks::RAISE_ASSERT);
const bool checkAllBlocks = hasFlag(checks, ProfileChecks::CHECK_ALL_BLOCKS);

Expand Down Expand Up @@ -5478,6 +5419,10 @@ void Compiler::fgDebugCheckProfileWeights(ProfileChecks checks)
JITDUMP("Profile is self-consistent (%d profiled blocks, %d unprofiled)\n", profiledBlocks,
unprofiledBlocks);
}
else if (verifyLikelihoodSum)
{
JITDUMP("All block successor flow edge likelihoods sum to 1.0\n");
}
else if (verifyHasLikelihood)
{
JITDUMP("All flow edges have likelihoods\n");
Expand Down Expand Up @@ -5618,10 +5563,10 @@ bool Compiler::fgDebugCheckIncomingProfileData(BasicBlock* block, ProfileChecks
bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks checks)
{
const bool verifyClassicWeights = fgEdgeWeightsComputed && hasFlag(checks, ProfileChecks::CHECK_CLASSIC);
const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY);
const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD);
const bool verifyLikelihoodSum = hasFlag(checks, ProfileChecks::CHECK_LIKELIHOODSUM);

if (!(verifyClassicWeights || verifyLikelyWeights || verifyHasLikelihood))
if (!(verifyClassicWeights || verifyHasLikelihood || verifyLikelihoodSum))
{
return true;
}
Expand Down Expand Up @@ -5711,7 +5656,7 @@ bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks
}
}

if (verifyLikelyWeights)
if (verifyLikelihoodSum)
{
if (!fgProfileWeightsConsistent(outgoingLikelihood, 1.0))
{
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7276,6 +7276,9 @@ void Compiler::impImportBlockCode(BasicBlock* block)
// to set BBF_NONE_QUIRK
block->SetFlags(BBF_NONE_QUIRK);
}

FlowEdge* const edge = fgGetPredForBlock(block->GetTarget(), block);
edge->setLikelihood(1.0);
}

break;
Expand Down Expand Up @@ -7537,6 +7540,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
{
// transform the basic block into a BBJ_ALWAYS
block->SetKindAndTargetEdge(BBJ_ALWAYS, curEdge);
curEdge->setLikelihood(1.0);
foundVal = true;
}
else
Expand Down
Loading

0 comments on commit 54b4a60

Please sign in to comment.