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: Continue profile consistency checks until after finally cloning #109792

Merged
merged 8 commits into from
Nov 19, 2024
Merged

JIT: Continue profile consistency checks until after finally cloning #109792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3029573
Move checks after fgAddInternal
amanasifkhalid Nov 6, 2024
5f941dd
wip
amanasifkhalid Nov 11, 2024
cbad712
Move checks after fgCloneFinally
amanasifkhalid Nov 13, 2024
52d8902
Remove commented out code
amanasifkhalid Nov 13, 2024
088503b
Merge from main
amanasifkhalid Nov 13, 2024
218beba
Style
amanasifkhalid Nov 13, 2024
8deddc7
Mark profile inconsistent when removing empty static alloc
amanasifkhalid Nov 13, 2024
6f73790
Merge branch 'main' into profile-checks-post-inlining
amanasifkhalid Nov 18, 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
10 changes: 5 additions & 5 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4609,11 +4609,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
// Record "start" values for post-inlining cycles and elapsed time.
RecordStateAtEndOfInlining();

// Drop back to just checking profile likelihoods.
//
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;
activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS;

if (opts.OptimizationEnabled())
{
// Build post-order and remove dead blocks
Expand Down Expand Up @@ -4658,6 +4653,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
//
DoPhase(this, PHASE_CLONE_FINALLY, &Compiler::fgCloneFinally);

// Drop back to just checking profile likelihoods.
//
activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE;
activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS;

// Do some flow-related optimizations
//
if (opts.OptimizationEnabled())
Expand Down
23 changes: 21 additions & 2 deletions src/coreclr/jit/fgehopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,8 +1038,27 @@ PhaseStatus Compiler::fgCloneFinally()
const unsigned finallyTryIndex = firstBlock->bbTryIndex;
BasicBlock* insertAfter = nullptr;
BlockToBlockMap blockMap(getAllocator());
unsigned cloneBBCount = 0;
weight_t const originalWeight = firstBlock->hasProfileWeight() ? firstBlock->bbWeight : BB_ZERO_WEIGHT;
unsigned cloneBBCount = 0;
weight_t originalWeight;

// When distributing weight between the original and cloned regions,
// ensure only weight from region entries is considered.
// Flow from loop backedges within the region should not influence the weight distribution ratio.
if (firstBlock->hasProfileWeight())
{
originalWeight = firstBlock->bbWeight;
for (BasicBlock* const predBlock : firstBlock->PredBlocks())
{
if (!predBlock->KindIs(BBJ_CALLFINALLY))
{
originalWeight = max(0.0, originalWeight - predBlock->bbWeight);
}
}
}
else
{
originalWeight = BB_ZERO_WEIGHT;
}

for (BasicBlock* block = firstBlock; block != nextBlock; block = block->Next())
{
Expand Down
7 changes: 0 additions & 7 deletions src/coreclr/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1401,15 +1401,8 @@ void Compiler::fgAddSyncMethodEnterExit()
// Create a block for the start of the try region, where the monitor enter call
// will go.
BasicBlock* const tryBegBB = fgSplitBlockAtEnd(fgFirstBB);
BasicBlock* const tryNextBB = tryBegBB->Next();
BasicBlock* const tryLastBB = fgLastBB;

// If we have profile data the new block will inherit the next block's weight
if (tryNextBB->hasProfileWeight())
{
tryBegBB->inheritWeight(tryNextBB);
}

// Create a block for the fault.
// It gets an artificial ref count.

Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/objectalloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,9 @@ unsigned int ObjectAllocator::MorphAllocObjNodeIntoStackAlloc(
if (predBlock->hasProfileWeight())
{
block->setBBProfileWeight(predBlock->bbWeight);
JITDUMP("Profile weight into " FMT_BB " needs to be propagated to successors. Profile %s inconsistent.\n",
block->bbNum, comp->fgPgoConsistent ? "is now" : "was already");
comp->fgPgoConsistent = false;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I should try harder to fix this one up. With PGO in most cases this path will have zero weight.

Copy link
Member Author

@amanasifkhalid amanasifkhalid Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In my experience, this path triggered consistency checks pretty rarely, and I expect that we'll want to run profile repair after the first flowgraph simplification pass for now, so this might trivially fix itself.

}

// Just lop off the JTRUE, the rest can clean up later
Expand Down
Loading