Skip to content

JIT: Remove fgFirstColdBlock checks in frontend phases #110452

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

Merged
amanasifkhalid merged 1 commit into from
Dec 6, 2024
Merged

JIT: Remove fgFirstColdBlock checks in frontend phases #110452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ bool BasicBlock::IsFirstColdBlock(Compiler* compiler) const
bool BasicBlock::CanRemoveJumpToNext(Compiler* compiler) const
{
assert(KindIs(BBJ_ALWAYS));
return JumpsToNext() && (bbNext != compiler->fgFirstColdBlock);
return JumpsToNext() && !IsLastHotBlock(compiler);
}

//------------------------------------------------------------------------
Expand All @@ -441,7 +441,7 @@ bool BasicBlock::CanRemoveJumpToTarget(BasicBlock* target, Compiler* compiler) c
{
assert(KindIs(BBJ_COND));
assert(TrueTargetIs(target) || FalseTargetIs(target));
return NextIs(target) && !compiler->fgInDifferentRegions(this, target);
return NextIs(target) && !IsLastHotBlock(compiler);
}

#ifdef DEBUG
Expand Down
26 changes: 7 additions & 19 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4945,7 +4945,7 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ)

// Removes the block from the bbPrev/bbNext chain
// Updates fgFirstBB and fgLastBB if necessary
// Does not update fgFirstFuncletBB or fgFirstColdBlock (fgUnlinkRange does)
// Does not update fgFirstFuncletBB
void Compiler::fgUnlinkBlock(BasicBlock* block)
{
if (block->IsFirst())
Expand Down Expand Up @@ -5006,6 +5006,9 @@ void Compiler::fgUnlinkRange(BasicBlock* bBeg, BasicBlock* bEnd)
assert(bBeg != nullptr);
assert(bEnd != nullptr);

// We shouldn't be churning the flowgraph after doing hot/cold splitting
assert(fgFirstColdBlock == nullptr);

BasicBlock* bPrev = bBeg->Prev();
assert(bPrev != nullptr); // Can't unlink a range starting with the first block

Expand All @@ -5020,12 +5023,6 @@ void Compiler::fgUnlinkRange(BasicBlock* bBeg, BasicBlock* bEnd)
bPrev->SetNext(bEnd->Next());
}

// If bEnd was the first Cold basic block update fgFirstColdBlock
if (bEnd->IsFirstColdBlock(this))
{
fgFirstColdBlock = bPrev->Next();
}

#ifdef DEBUG
if (UsesFunclets())
{
Expand Down Expand Up @@ -5056,6 +5053,9 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
{
assert(block != nullptr);

// We shouldn't churn the flowgraph after doing hot/cold splitting
assert(fgFirstColdBlock == nullptr);

JITDUMP("fgRemoveBlock " FMT_BB ", unreachable=%s\n", block->bbNum, dspBool(unreachable));

BasicBlock* bPrev = block->Prev();
Expand All @@ -5079,12 +5079,6 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
fgFirstFuncletBB = block->Next();
}

// If this is the first Cold basic block update fgFirstColdBlock
if (block->IsFirstColdBlock(this))
{
fgFirstColdBlock = block->Next();
}

// A BBJ_CALLFINALLY is usually paired with a BBJ_CALLFINALLYRET.
// If we delete such a BBJ_CALLFINALLY we also delete the BBJ_CALLFINALLYRET.
if (block->isBBCallFinallyPair())
Expand Down Expand Up @@ -5141,12 +5135,6 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
skipUnmarkLoop = true;
}

// If this is the first Cold basic block update fgFirstColdBlock
if (block->IsFirstColdBlock(this))
{
fgFirstColdBlock = block->Next();
}

// Update fgFirstFuncletBB if necessary
if (block == fgFirstFuncletBB)
{
Expand Down
47 changes: 11 additions & 36 deletions src/coreclr/jit/fgopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,13 +830,6 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block)
return false;
}

// We don't want to compact blocks that are in different hot/cold regions
//
if (fgInDifferentRegions(block, target))
{
return false;
}

// We cannot compact two blocks in different EH regions.
//
if (!BasicBlock::sameEHRegion(block, target))
Expand Down Expand Up @@ -872,6 +865,10 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block)
void Compiler::fgCompactBlock(BasicBlock* block)
{
assert(fgCanCompactBlock(block));

// We shouldn't churn the flowgraph after doing hot/cold splitting
assert(fgFirstColdBlock == nullptr);

BasicBlock* const target = block->GetTarget();

JITDUMP("\nCompacting " FMT_BB " into " FMT_BB ":\n", target->bbNum, block->bbNum);
Expand Down Expand Up @@ -1356,6 +1353,9 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block)
{
assert(block->isEmpty());

// We shouldn't churn the flowgraph after doing hot/cold splitting
assert(fgFirstColdBlock == nullptr);

bool madeChanges = false;
BasicBlock* bPrev = block->Prev();

Expand Down Expand Up @@ -1401,12 +1401,6 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block)
break;
}

// can't allow fall through into cold code
if (block->IsLastHotBlock(this))
Copy link
Contributor Author

@amanasifkhalid amanasifkhalid Dec 6, 2024

Choose a reason for hiding this comment

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

This check looks like it wouldn't pessimize anything if we aren't hot/cold splitting, but IsLastHotBlock returns true if block == fgLastBB too, so this check was blocking empty block removal for the last block in the worklist. Thus, the check's removal incurs some small diffs. PerfScore diffs look wonky due to churn in optSetBlockWeights, but the actual layout diffs look like an improvement.

{
break;
}

// Don't remove fgEntryBB
if (block == fgEntryBB)
{
Expand Down Expand Up @@ -5610,6 +5604,9 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */,

noway_assert(opts.OptimizationEnabled());

// We shouldn't be churning the flowgraph after doing hot/cold splitting
assert(fgFirstColdBlock == nullptr);

#ifdef DEBUG
if (verbose && !isPhase)
{
Expand Down Expand Up @@ -5766,9 +5763,7 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */,
bNext->KindIs(BBJ_ALWAYS) && // the next block is a BBJ_ALWAYS block
!bNext->JumpsToNext() && // and it doesn't jump to the next block (we might compact them)
bNext->isEmpty() && // and it is an empty block
!bNext->TargetIs(bNext) && // special case for self jumps
!bDest->IsFirstColdBlock(this) &&
!fgInDifferentRegions(block, bDest)) // do not cross hot/cold sections
!bNext->TargetIs(bNext)) // special case for self jumps
{
assert(block->FalseTargetIs(bNext));

Expand Down Expand Up @@ -5812,20 +5807,6 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */,
optimizeJump = false;
}

// If we are optimizing using real profile weights
// then don't optimize a conditional jump to an unconditional jump
// until after we have computed the edge weights
//
if (fgIsUsingProfileWeights())
{
// if block and bDest are in different hot/cold regions we can't do this optimization
// because we can't allow fall-through into the cold region.
if (fgInDifferentRegions(block, bDest))
{
optimizeJump = false;
}
}

if (optimizeJump && isJumpToJoinFree)
{
// In the join free case, we also need to move bDest right after bNext
Expand Down Expand Up @@ -5914,12 +5895,6 @@ bool Compiler::fgUpdateFlowGraph(bool doTailDuplication /* = false */,
/* Mark the block as removed */
bNext->SetFlags(BBF_REMOVED);

// If this is the first Cold basic block update fgFirstColdBlock
if (bNext->IsFirstColdBlock(this))
{
fgFirstColdBlock = bNext->Next();
}

//
// If we removed the end of a try region or handler region
// we will need to update ebdTryLast or ebdHndLast.
Expand Down
Loading