Skip to content

Commit

Permalink
JIT: Ensure edge likelihoods 1/N (#97740)
Browse files Browse the repository at this point in the history 
Start enforcing that all FlowEdges have likelihoods set (ideally, to the right value).

Update the profile checker to allow checking just for likelihoods. Enable this for all the initial phases up through and including fgAddInternal.

Fix missing flow edge issues by generally having whatever bit of code calls `fgAddRef` also ensure likelihood is set. For many cases these are unconditional flows so the value is just 1.0. Sometimes we can pass the old edge in and have`fgAddRef` itself do the work. And in a few cases (switches) things are a bit more subtle.

The general plan is to leave things like this for now until we've fixed likelihoods everywhere and are checking all phases. Then we'll go back and try and refactor `fgAddRef` to do more of this automatically.

Contributes to #93020
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Feb 1, 2024
1 parent 0fff4ef commit 5cee418
Show file tree
Hide file tree
Showing 14 changed files with 248 additions and 122 deletions.
16 changes: 7 additions & 9 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2070,9 +2070,8 @@ struct FlowEdge
// The count of duplicate "edges" (used for switch stmts or degenerate branches)
unsigned m_dupCount;

#ifdef DEBUG
// True if likelihood has been set
bool m_likelihoodSet;
#endif

public:
FlowEdge(BasicBlock* block, FlowEdge* rest)
Expand All @@ -2081,7 +2080,8 @@ struct FlowEdge
, m_edgeWeightMin(0)
, m_edgeWeightMax(0)
, m_likelihood(0)
, m_dupCount(0) DEBUGARG(m_likelihoodSet(false))
, m_dupCount(0)
, m_likelihoodSet(false)
{
}

Expand Down Expand Up @@ -2137,22 +2137,20 @@ struct FlowEdge
{
assert(likelihood >= 0.0);
assert(likelihood <= 1.0);
INDEBUG(m_likelihoodSet = true);
m_likelihood = likelihood;
m_likelihoodSet = true;
m_likelihood = likelihood;
}

void clearLikelihood()
{
m_likelihood = 0.0;
INDEBUG(m_likelihoodSet = false);
m_likelihood = 0.0;
m_likelihoodSet = false;
}

#ifdef DEBUG
bool hasLikelihood() const
{
return m_likelihoodSet;
}
#endif

weight_t getLikelyWeight() const
{
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4503,7 +4503,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
activePhaseChecks |= PhaseChecks::CHECK_PROFILE;
DoPhase(this, PHASE_INCPROFILE, &Compiler::fgIncorporateProfileData);
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;

// If we are doing OSR, update flow to initially reach the appropriate IL offset.
//
Expand Down Expand Up @@ -4606,6 +4605,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
DoPhase(this, PHASE_MORPH_ADD_INTERNAL, &Compiler::fgAddInternal);

// Disable profile checks now.
// Over time we will move this further and further back in the phase list, as we fix issues.
//
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;

// Remove empty try regions
//
DoPhase(this, PHASE_EMPTY_TRY, &Compiler::fgRemoveEmptyTry);
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 @@ -1580,9 +1580,10 @@ enum class ProfileChecks : unsigned int
{
CHECK_NONE = 0,
CHECK_CLASSIC = 1 << 0, // check "classic" jit weights
CHECK_LIKELY = 1 << 1, // check likelihood based weights
RAISE_ASSERT = 1 << 2, // assert on check failure
CHECK_ALL_BLOCKS = 1 << 3, // check blocks even if bbHasProfileWeight is false
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
};

inline constexpr ProfileChecks operator ~(ProfileChecks a)
Expand Down
68 changes: 53 additions & 15 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,17 +390,29 @@ void Compiler::fgChangeSwitchBlock(BasicBlock* oldSwitchBlock, BasicBlock* newSw
// fgRemoveRefPred()/fgAddRefPred() will do the right thing: the second and
// subsequent duplicates will simply subtract from and add to the duplicate
// count (respectively).

//
// However this does the "wrong" thing with respect to edge profile
// data; the old edge is not returned by fgRemoveRefPred until it has
// a dup count of 0, and the fgAddRefPred only uses the optional
// old edge arg when the new edge is first created.
//
// Remove the old edge [oldSwitchBlock => bJump]
//
assert(bJump->countOfInEdges() > 0);
fgRemoveRefPred(bJump, oldSwitchBlock);
FlowEdge* const oldEdge = fgRemoveRefPred(bJump, oldSwitchBlock);

//
// Create the new edge [newSwitchBlock => bJump]
//
fgAddRefPred(bJump, newSwitchBlock);
FlowEdge* const newEdge = fgAddRefPred(bJump, newSwitchBlock);

// Handle the profile update, once we get our hands on the old edge.
//
if (oldEdge != nullptr)
{
assert(!newEdge->hasLikelihood());
newEdge->setLikelihood(oldEdge->getLikelihood());
}
}

if (m_switchDescMap != nullptr)
Expand Down Expand Up @@ -608,10 +620,13 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE: // This function can be called before import, so we still have BBJ_LEAVE
assert(block->TargetIs(oldTarget));
block->SetTarget(newTarget);
fgRemoveRefPred(oldTarget, block);
fgAddRefPred(newTarget, block);

if (block->TargetIs(oldTarget))
{
block->SetTarget(newTarget);
FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block);
fgAddRefPred(newTarget, block, oldEdge);
}
break;

case BBJ_COND:
Expand All @@ -637,7 +652,15 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas

// TODO-NoFallThrough: Proliferate weight from oldEdge
// (as a quirk, we avoid doing so for the true target to reduce diffs for now)
fgAddRefPred(newTarget, block);
FlowEdge* const newEdge = fgAddRefPred(newTarget, block);
if (block->KindIs(BBJ_ALWAYS))
{
newEdge->setLikelihood(1.0);
}
else if (oldEdge->hasLikelihood())
{
newEdge->setLikelihood(oldEdge->getLikelihood());
}
}
else
{
Expand All @@ -661,10 +684,21 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas
{
if (jumpTab[i] == oldTarget)
{
jumpTab[i] = newTarget;
changed = true;
fgRemoveRefPred(oldTarget, block);
fgAddRefPred(newTarget, block);
jumpTab[i] = newTarget;
changed = true;
FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block);
FlowEdge* const newEdge = fgAddRefPred(newTarget, block, oldEdge);

// Handle the profile update, once we get our hands on the old edge.
// (see notes in fgChangeSwitchBlock for why this extra step is necessary)
//
// We do it slightly differently here so we don't lose the old
// edge weight propagation that would sometimes happen
//
if ((oldEdge != nullptr) && !newEdge->hasLikelihood())
{
newEdge->setLikelihood(oldEdge->getLikelihood());
}
}
}

Expand Down Expand Up @@ -4739,7 +4773,8 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
curr->SetFlags(BBF_NONE_QUIRK);
assert(curr->JumpsToNext());

fgAddRefPred(newBlock, curr);
FlowEdge* const newEdge = fgAddRefPred(newBlock, curr);
newEdge->setLikelihood(1.0);

return newBlock;
}
Expand Down Expand Up @@ -4981,12 +5016,15 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)
// newBlock replaces succ as curr's successor.
fgReplaceJumpTarget(curr, succ, newBlock);

// And succ has newBlock as a new predecessor.
fgAddRefPred(succ, newBlock);
// And 'succ' has 'newBlock' as a new predecessor.
FlowEdge* const newEdge = fgAddRefPred(succ, newBlock);
newEdge->setLikelihood(1.0);

// This isn't accurate, but it is complex to compute a reasonable number so just assume that we take the
// branch 50% of the time.
//
// TODO: leverage edge likelihood.
//
if (!curr->KindIs(BBJ_ALWAYS))
{
newBlock->inheritWeightPercentage(curr, 50);
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/jit/fgflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,13 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE
block->bbLastPred = flow;
}

// Copy likelihood from old edge, if any.
//
if ((oldEdge != nullptr) && oldEdge->hasLikelihood())
{
flow->setLikelihood(oldEdge->getLikelihood());
}

if (fgHaveValidEdgeWeights)
{
// We are creating an edge from blockPred to block
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/jit/fginline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1534,7 +1534,8 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
bottomBlock->bbNum);

block->SetKindAndTarget(BBJ_ALWAYS, bottomBlock);
fgAddRefPred(bottomBlock, block);
FlowEdge* const newEdge = fgAddRefPred(bottomBlock, block);
newEdge->setLikelihood(1.0);

if (block == InlineeCompiler->fgLastBB)
{
Expand All @@ -1553,8 +1554,8 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
topBlock->SetNext(InlineeCompiler->fgFirstBB);
topBlock->SetTarget(topBlock->Next());
topBlock->SetFlags(BBF_NONE_QUIRK);
fgRemoveRefPred(bottomBlock, topBlock);
fgAddRefPred(InlineeCompiler->fgFirstBB, topBlock);
FlowEdge* const oldEdge = fgRemoveRefPred(bottomBlock, topBlock);
fgAddRefPred(InlineeCompiler->fgFirstBB, topBlock, oldEdge);
InlineeCompiler->fgLastBB->SetNext(bottomBlock);

//
Expand Down
18 changes: 14 additions & 4 deletions src/coreclr/jit/fgopt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ PhaseStatus Compiler::fgPostImportationCleanup()
}
else
{
fgAddRefPred(newTryEntry->Next(), newTryEntry);
FlowEdge* const newEdge = fgAddRefPred(newTryEntry->Next(), newTryEntry);
newEdge->setLikelihood(1.0);
}

JITDUMP("OSR: changing start of try region #%u from " FMT_BB " to new " FMT_BB "\n",
Expand Down Expand Up @@ -773,6 +774,7 @@ PhaseStatus Compiler::fgPostImportationCleanup()
fromBlock->SetFlags(BBF_INTERNAL);
newBlock->RemoveFlags(BBF_DONT_REMOVE);
addedBlocks++;
FlowEdge* const normalTryEntryEdge = fgGetPredForBlock(newBlock, fromBlock);

GenTree* const entryStateLcl = gtNewLclvNode(entryStateVar, TYP_INT);
GenTree* const compareEntryStateToZero =
Expand All @@ -781,9 +783,17 @@ PhaseStatus Compiler::fgPostImportationCleanup()
fgNewStmtAtBeg(fromBlock, jumpIfEntryStateZero);

fromBlock->SetCond(toBlock, newBlock);
fgAddRefPred(toBlock, fromBlock);
FlowEdge* const osrTryEntryEdge = fgAddRefPred(toBlock, fromBlock);
newBlock->inheritWeight(fromBlock);

// Not sure what the correct edge likelihoods are just yet;
// for now we'll say the OSR path is the likely one.
//
// Todo: can we leverage profile data here to get a better answer?
//
osrTryEntryEdge->setLikelihood(0.9);
normalTryEntryEdge->setLikelihood(0.1);

entryJumpTarget = fromBlock;
};

Expand Down Expand Up @@ -824,8 +834,8 @@ PhaseStatus Compiler::fgPostImportationCleanup()
if (entryJumpTarget != osrEntry)
{
fgFirstBB->SetTarget(entryJumpTarget);
fgRemoveRefPred(osrEntry, fgFirstBB);
fgAddRefPred(entryJumpTarget, fgFirstBB);
FlowEdge* const oldEdge = fgRemoveRefPred(osrEntry, fgFirstBB);
fgAddRefPred(entryJumpTarget, fgFirstBB, oldEdge);

JITDUMP("OSR: redirecting flow from method entry " FMT_BB " to OSR entry " FMT_BB
" via step blocks.\n",
Expand Down
Loading

0 comments on commit 5cee418

Please sign in to comment.