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

JIT: start checking sum of successor likelihoods #99024

Merged
merged 6 commits into from
Feb 29, 2024
Merged

JIT: start checking sum of successor likelihoods #99024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64b4091
JIT: start checking sum of successor likelihoods
AndyAyersMS Feb 27, 2024
3159276
move method so it's available in release
AndyAyersMS Feb 28, 2024
b614220
Merge branch 'main' into ConsistentEdgeLikelihoods_1
AndyAyersMS Feb 28, 2024
490ab8a
post-merge cleanup, also rework last bit of multi-guess gdv
AndyAyersMS Feb 28, 2024
e4a83a8
fix chained likelhood example computation
AndyAyersMS Feb 28, 2024
753a207
adjust likelihood adjustment
AndyAyersMS Feb 28, 2024
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
18 changes: 18 additions & 0 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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);
AndyAyersMS marked this conversation as resolved.
Show resolved Hide resolved
edge->setLikelihood(1.0);
}
}
else
Expand Down
121 changes: 33 additions & 88 deletions src/coreclr/jit/fgprofile.cpp
View file
Open in desktop
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
Open in desktop
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
Loading