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: profile updates for return merges and tail calls #48773

Merged
merged 7 commits into from
Mar 2, 2021
Merged

JIT: profile updates for return merges and tail calls #48773

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d78d2e7
JIT: profile updates for return merges and tail calls
AndyAyersMS Feb 25, 2021
ccac6e4
Fix non-PGO return merge weights (basically, don't set them).
AndyAyersMS Feb 25, 2021
899dda1
one more fix for common return block
AndyAyersMS Feb 25, 2021
33870e6
Properly update edge weights when creating a preheader.
AndyAyersMS Feb 26, 2021
581f923
Handle the more complex tail call profile fixup where two blocks must…
AndyAyersMS Feb 26, 2021
a8bda5f
review feedback
AndyAyersMS Mar 2, 2021
7569715
refactor setBBProfileWeight
AndyAyersMS Mar 2, 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
15 changes: 12 additions & 3 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ typedef BitVec_ValRet_T ASSERT_VALRET_TP;
// This define is used with string concatenation to put this in printf format strings (Note that %u means unsigned int)
#define FMT_BB "BB%02u"

// And this format for profile weights
#define FMT_WT "%.7g"

/*****************************************************************************
*
* Each basic block ends with a jump which is described as a value
Expand Down Expand Up @@ -1291,9 +1294,15 @@ struct flowList
// They return false if the newWeight is not between the current [min..max]
// when slop is non-zero we allow for the case where our weights might be off by 'slop'
//
bool setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, BasicBlock::weight_t slop, bool* wbUsedSlop);
bool setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight, BasicBlock::weight_t slop, bool* wbUsedSlop);
void setEdgeWeights(BasicBlock::weight_t newMinWeight, BasicBlock::weight_t newMaxWeight);
bool setEdgeWeightMinChecked(BasicBlock::weight_t newWeight,
BasicBlock* bDst,
BasicBlock::weight_t slop,
bool* wbUsedSlop);
bool setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight,
BasicBlock* bDst,
BasicBlock::weight_t slop,
bool* wbUsedSlop);
void setEdgeWeights(BasicBlock::weight_t newMinWeight, BasicBlock::weight_t newMaxWeight, BasicBlock* bDst);

flowList(BasicBlock* block, flowList* rest)
: flNext(rest), m_block(block), flEdgeWeightMin(0), flEdgeWeightMax(0), flDupCount(0)
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/jit/fgflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ flowList* Compiler::fgAddRefPred(BasicBlock* block,
// If our caller has given us the old edge weights
// then we will use them.
//
flow->setEdgeWeights(oldEdge->edgeWeightMin(), oldEdge->edgeWeightMax());
flow->setEdgeWeights(oldEdge->edgeWeightMin(), oldEdge->edgeWeightMax(), block);
}
else
{
Expand All @@ -284,17 +284,17 @@ flowList* Compiler::fgAddRefPred(BasicBlock* block,
// otherwise it is the same as the edge's max weight.
if (blockPred->NumSucc() > 1)
{
flow->setEdgeWeights(BB_ZERO_WEIGHT, newWeightMax);
flow->setEdgeWeights(BB_ZERO_WEIGHT, newWeightMax, block);
}
else
{
flow->setEdgeWeights(flow->edgeWeightMax(), newWeightMax);
flow->setEdgeWeights(flow->edgeWeightMax(), newWeightMax, block);
}
}
}
else
{
flow->setEdgeWeights(BB_ZERO_WEIGHT, BB_MAX_WEIGHT);
flow->setEdgeWeights(BB_ZERO_WEIGHT, BB_MAX_WEIGHT, block);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/fgopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2323,7 +2323,7 @@ bool Compiler::fgOptimizeBranchToEmptyUnconditional(BasicBlock* block, BasicBloc
{
newEdge2Max = BB_ZERO_WEIGHT;
}
edge2->setEdgeWeights(newEdge2Min, newEdge2Max);
edge2->setEdgeWeights(newEdge2Min, newEdge2Max, bDest);
}
}

Expand Down
178 changes: 126 additions & 52 deletions src/coreclr/jit/fgprofile.cpp
View file
Open in desktop

Large diffs are not rendered by default.

61 changes: 29 additions & 32 deletions src/coreclr/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2313,17 +2313,7 @@ class MergedReturns

noway_assert(newReturnBB->bbNext == nullptr);

#ifdef DEBUG
if (comp->verbose)
{
printf("\n newReturnBB [" FMT_BB "] created\n", newReturnBB->bbNum);
}
#endif

// We have profile weight, the weight is zero, and the block is run rarely,
// until we prove otherwise by merging other returns into this one.
newReturnBB->bbFlags |= (BBF_PROF_WEIGHT | BBF_RUN_RARELY);
newReturnBB->bbWeight = 0;
JITDUMP("\n newReturnBB [" FMT_BB "] created\n", newReturnBB->bbNum);

GenTree* returnExpr;

Expand Down Expand Up @@ -2505,13 +2495,41 @@ class MergedReturns
// merged return block are lexically forward.

insertionPoints[index] = returnBlock;

// Update profile information in the mergedReturnBlock to
// reflect the additional flow.
//
if (returnBlock->hasProfileWeight())
{
BasicBlock::weight_t const oldWeight =
mergedReturnBlock->hasProfileWeight() ? mergedReturnBlock->bbWeight : 0.0f;
BasicBlock::weight_t const newWeight = oldWeight + returnBlock->bbWeight;

JITDUMP("merging profile weight %.6f from " FMT_BB " to const return " FMT_BB "\n",
AndyAyersMS marked this conversation as resolved.
Show resolved Hide resolved
returnBlock->bbWeight, returnBlock->bbNum, mergedReturnBlock->bbNum);

mergedReturnBlock->setBBProfileWeight(newWeight);

if (newWeight > 0.0f)
{
mergedReturnBlock->bbFlags &= ~BBF_RUN_RARELY;
}
else
{
mergedReturnBlock->bbFlags |= BBF_RUN_RARELY;
}

DISPBLOCK(mergedReturnBlock);
}
}
}
}

if (mergedReturnBlock == nullptr)
{
// No constant return block for this return; use the general one.
// We defer flow update and profile update to morph.
//
mergedReturnBlock = comp->genReturnBB;
if (mergedReturnBlock == nullptr)
{
Expand All @@ -2528,20 +2546,6 @@ class MergedReturns

if (returnBlock != nullptr)
{
// Propagate profile weight and related annotations to the merged block.
// Return weight should never exceed entry weight, so cap it to avoid nonsensical
// hot returns in synthetic profile settings.
mergedReturnBlock->bbWeight =
min(mergedReturnBlock->bbWeight + returnBlock->bbWeight, comp->fgFirstBB->bbWeight);
if (!returnBlock->hasProfileWeight())
{
mergedReturnBlock->bbFlags &= ~BBF_PROF_WEIGHT;
}
if (mergedReturnBlock->bbWeight > 0)
{
mergedReturnBlock->bbFlags &= ~BBF_RUN_RARELY;
}

// Update fgReturnCount to reflect or anticipate that `returnBlock` will no longer
// be a return point.
comp->fgReturnCount--;
Expand Down Expand Up @@ -2746,13 +2750,6 @@ void Compiler::fgAddInternal()
// will expect to find it.
BasicBlock* mergedReturn = merger.EagerCreate();
assert(mergedReturn == genReturnBB);
// Assume weight equal to entry weight for this BB.
mergedReturn->bbFlags &= ~BBF_PROF_WEIGHT;
mergedReturn->bbWeight = fgFirstBB->bbWeight;
if (mergedReturn->bbWeight > 0)
{
mergedReturn->bbFlags &= ~BBF_RUN_RARELY;
}
}
else
{
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/jit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,9 @@ const bool dspGCtbls = true;
#define DISPTREERANGE(range, t) \
if (JitTls::GetCompiler()->verbose) \
JitTls::GetCompiler()->gtDispTreeRange(range, t);
#define DISPBLOCK(b) \
if (JitTls::GetCompiler()->verbose) \
JitTls::GetCompiler()->fgTableDispBasicBlock(b);
#define VERBOSE JitTls::GetCompiler()->verbose
#else // !DEBUG
#define JITDUMP(...)
Expand All @@ -559,6 +562,7 @@ const bool dspGCtbls = true;
#define DISPSTMT(t)
#define DISPRANGE(range)
#define DISPTREERANGE(range, t)
#define DISPBLOCK(b)
#define VERBOSE 0
#endif // !DEBUG

Expand Down
136 changes: 122 additions & 14 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7637,10 +7637,100 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call)
}
#endif

// This block is not longer any block's predecessor. If we end up
// converting this tail call to a branch, we'll add appropriate
// successor information then.
fgRemoveBlockAsPred(compCurBB);
// If this block has a flow successor, make suitable updates.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if all this new code (or some of it?) should be extracted to a fgUpdateTailcallProfileWeights or similar?

In fact, I wonder if most/all of the profile weight maintenance should be extracted into functions in fgprofile.cpp instead of spread around the various bits of the code base.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think it belongs with the transformation; this is an important part of the transformation and not some additional / optional side work.

In the (hopefully not too distant) future we'll always have profile weights on every block so some of the conditional guarding will go way.

//
BasicBlock* const nextBlock = compCurBB->GetUniqueSucc();

if (nextBlock == nullptr)
{
// No unique successor. compCurBB should be a return.
//
assert(compCurBB->bbJumpKind == BBJ_RETURN);
}
else
{
// Flow no longer reaches nextBlock from here.
//
fgRemoveRefPred(nextBlock, compCurBB);

// Adjust profile weights.
//
// Note if this is a tail call to loop, further updates
// are needed once we install the loop edge.
//
if (compCurBB->hasProfileWeight() && nextBlock->hasProfileWeight())
{
// Since we have linear flow we can update the next block weight.
//
BasicBlock::weight_t const blockWeight = compCurBB->bbWeight;
BasicBlock::weight_t const nextWeight = nextBlock->bbWeight;
BasicBlock::weight_t const newNextWeight = nextWeight - blockWeight;

// If the math would result in an negative weight then there's
AndyAyersMS marked this conversation as resolved.
Show resolved Hide resolved
// no local repair we can do; just leave things inconsistent.
//
if (newNextWeight >= 0)
{
// Note if we'd already morphed the IR in nextblock we might
// have done something profile sensitive that we should arguably reconsider.
//
JITDUMP("Reducing profile weight of " FMT_BB " from " FMT_WT " to " FMT_WT "\n", nextBlock->bbNum,
nextWeight, newNextWeight);

nextBlock->setBBProfileWeight(newNextWeight);

if (newNextWeight == 0.0f)
Copy link
Member

Choose a reason for hiding this comment

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

Should all these 0.0f constants be some ZERO_WEIGHT define?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the relevant define is BB_ZERO_WEIGHT. A quick regex suggests it's almost exactly as popular as the 0 literal when used for comparisons with bbWeight.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm somewhat ambivalent about the value of having BB_ZERO_WEIGHT and BB_UNITY_WEIGHT since going forward we're quite unlikely to ever change them from 0.0f and 1.0f.

But I suppose we should be consistent and perhaps the symbolic form calls more attention to itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed what I could easily find over to BB_ZERO_WEIGHT.

{
nextBlock->bbFlags |= BBF_RUN_RARELY;
}
}
else
{
JITDUMP("Not reducing profile weight of " FMT_BB " as its weight " FMT_WT
" is less than direct flow pred " FMT_BB " weight " FMT_WT "\n",
nextBlock->bbNum, nextWeight, compCurBB->bbNum, blockWeight);
}

// If nextBlock is not a BBJ_RETURN, it should have a unique successor that
// is a BBJ_RETURN, as we allow a little bit of flow after a tail call.
//
if (nextBlock->bbJumpKind != BBJ_RETURN)
{
BasicBlock* const nextNextBlock = nextBlock->GetUniqueSucc();
assert(nextNextBlock->bbJumpKind == BBJ_RETURN);

if (nextNextBlock->hasProfileWeight())
{
// Do similar updates here.
//
BasicBlock::weight_t const nextNextWeight = nextNextBlock->bbWeight;
BasicBlock::weight_t const newNextNextWeight = nextNextWeight - blockWeight;

// If the math would result in an negative weight then there's
// no local repair we can do; just leave things inconsistent.
//
if (newNextNextWeight >= 0)
{
JITDUMP("Reducing profile weight of " FMT_BB " from " FMT_WT " to " FMT_WT "\n",
nextNextBlock->bbNum, nextNextWeight, newNextNextWeight);

nextNextBlock->setBBProfileWeight(newNextNextWeight);

if (newNextNextWeight == 0.0f)
{
nextNextBlock->bbFlags |= BBF_RUN_RARELY;
}
}
else
{
JITDUMP("Not reducing profile weight of " FMT_BB " as its weight " FMT_WT
" is less than direct flow pred " FMT_BB " weight " FMT_WT "\n",
nextNextBlock->bbNum, nextNextWeight, compCurBB->bbNum, blockWeight);
}
}
}
}
}

#if !FEATURE_TAILCALL_OPT_SHARED_RETURN
// We enable shared-ret tail call optimization for recursive calls even if
Expand Down Expand Up @@ -16273,7 +16363,7 @@ bool Compiler::fgFoldConditional(BasicBlock* block)
{
// The edge weights for (block -> bTaken) are 100% of block's weight

edgeTaken->setEdgeWeights(block->bbWeight, block->bbWeight);
edgeTaken->setEdgeWeights(block->bbWeight, block->bbWeight, bTaken);

if (!bTaken->hasProfileWeight())
{
Expand All @@ -16290,7 +16380,7 @@ bool Compiler::fgFoldConditional(BasicBlock* block)
if (bTaken->countOfInEdges() == 1)
{
// There is only one in edge to bTaken
edgeTaken->setEdgeWeights(bTaken->bbWeight, bTaken->bbWeight);
edgeTaken->setEdgeWeights(bTaken->bbWeight, bTaken->bbWeight, bTaken);

// Update the weight of block
block->inheritWeight(bTaken);
Expand All @@ -16311,21 +16401,21 @@ bool Compiler::fgFoldConditional(BasicBlock* block)
edge = fgGetPredForBlock(bUpdated->bbNext, bUpdated);
newMaxWeight = bUpdated->bbWeight;
newMinWeight = min(edge->edgeWeightMin(), newMaxWeight);
edge->setEdgeWeights(newMinWeight, newMaxWeight);
edge->setEdgeWeights(newMinWeight, newMaxWeight, bUpdated->bbNext);
break;

case BBJ_COND:
edge = fgGetPredForBlock(bUpdated->bbNext, bUpdated);
newMaxWeight = bUpdated->bbWeight;
newMinWeight = min(edge->edgeWeightMin(), newMaxWeight);
edge->setEdgeWeights(newMinWeight, newMaxWeight);
edge->setEdgeWeights(newMinWeight, newMaxWeight, bUpdated->bbNext);
FALLTHROUGH;

case BBJ_ALWAYS:
edge = fgGetPredForBlock(bUpdated->bbJumpDest, bUpdated);
newMaxWeight = bUpdated->bbWeight;
newMinWeight = min(edge->edgeWeightMin(), newMaxWeight);
edge->setEdgeWeights(newMinWeight, newMaxWeight);
edge->setEdgeWeights(newMinWeight, newMaxWeight, bUpdated->bbNext);
break;

default:
Expand Down Expand Up @@ -17074,13 +17164,31 @@ void Compiler::fgMergeBlockReturn(BasicBlock* block)

fgRemoveStmt(block, lastStmt);
}
#ifdef DEBUG
if (verbose)

JITDUMP("\nUpdate " FMT_BB " to jump to common return block.\n", block->bbNum);
DISPBLOCK(block);

if (block->hasProfileWeight())
{
printf("morph " FMT_BB " to point at onereturn. New block is\n", block->bbNum);
fgTableDispBasicBlock(block);
BasicBlock::weight_t const oldWeight = genReturnBB->hasProfileWeight() ? genReturnBB->bbWeight : 0.0f;
BasicBlock::weight_t const newWeight = oldWeight + block->bbWeight;

JITDUMP("merging profile weight %.6f from " FMT_BB " to common return " FMT_BB "\n", block->bbWeight,
AndyAyersMS marked this conversation as resolved.
Show resolved Hide resolved
block->bbNum, genReturnBB->bbNum);

genReturnBB->setBBProfileWeight(newWeight);

if (newWeight > 0.0f)
{
genReturnBB->bbFlags &= ~BBF_RUN_RARELY;
}
else
{
genReturnBB->bbFlags |= BBF_RUN_RARELY;
}

DISPBLOCK(genReturnBB);
}
#endif
}
}

Expand Down
Loading