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: extract BBJ_COND to BBJ_ALWAYS profile repair as utility #110494

Merged
merged 2 commits into from
Dec 10, 2024
Merged

JIT: extract BBJ_COND to BBJ_ALWAYS profile repair as utility #110494

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17566c3
JIT: extract BBJ_COND to BBJ_ALWAYS profile repair as utility
AndyAyersMS Dec 6, 2024
5b04a6f
Apply suggestions from code review
AndyAyersMS Dec 9, 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
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6545,6 +6545,7 @@ class Compiler
}

void fgRemoveProfileData(const char* reason);
void fgRepairProfileCondToUncond(BasicBlock* block, FlowEdge* retainedEdge, FlowEdge* removedEdge, int* metric = nullptr);

//-------- Insert a statement at the start or end of a basic block --------

Expand Down
96 changes: 13 additions & 83 deletions src/coreclr/jit/fginline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,98 +644,28 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
m_compiler->gtUpdateNodeSideEffects(tree);
assert((tree->gtFlags & GTF_SIDE_EFFECT) == 0);
tree->gtBashToNOP();
m_madeChanges = true;
FlowEdge* removedEdge = nullptr;
m_madeChanges = true;
FlowEdge* removedEdge = nullptr;
FlowEdge* retainedEdge = nullptr;

if (condTree->IsIntegralConst(0))
{
removedEdge = block->GetTrueEdge();
m_compiler->fgRemoveRefPred(removedEdge);
block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetFalseEdge());
removedEdge = block->GetTrueEdge();
retainedEdge = block->GetFalseEdge();
}
else
{
removedEdge = block->GetFalseEdge();
m_compiler->fgRemoveRefPred(removedEdge);
block->SetKindAndTargetEdge(BBJ_ALWAYS, block->GetTrueEdge());
removedEdge = block->GetFalseEdge();
retainedEdge = block->GetTrueEdge();
}

// Update profile; make it consistent if possible
//
if (block->hasProfileWeight())
{
bool repairWasComplete = true;
bool missingProfileData = false;
weight_t const weight = removedEdge->getLikelyWeight();

if (weight > 0)
{
// Target block weight will increase.
//
BasicBlock* const target = block->GetTarget();

// We may have a profiled inlinee in an unprofiled method
//
if (target->hasProfileWeight())
{
target->setBBProfileWeight(target->bbWeight + weight);
missingProfileData = true;
}

// Alternate weight will decrease
//
BasicBlock* const alternate = removedEdge->getDestinationBlock();

if (alternate->hasProfileWeight())
{
weight_t const alternateNewWeight = alternate->bbWeight - weight;

// If profile weights are consistent, expect at worst a slight underflow.
//
if (m_compiler->fgPgoConsistent && (alternateNewWeight < 0))
{
assert(m_compiler->fgProfileWeightsEqual(alternateNewWeight, 0));
}
alternate->setBBProfileWeight(max(0.0, alternateNewWeight));
}
else
{
missingProfileData = true;
}

// This will affect profile transitively, so in general
// the profile will become inconsistent.
//
repairWasComplete = false;
m_compiler->fgRemoveRefPred(removedEdge);
block->SetKindAndTargetEdge(BBJ_ALWAYS, retainedEdge);

// But we can check for the special case where the
// block's postdominator is target's target (simple
// if/then/else/join).
//
if (!missingProfileData && target->KindIs(BBJ_ALWAYS))
{
repairWasComplete =
alternate->KindIs(BBJ_ALWAYS) && alternate->TargetIs(target->GetTarget());
}
}

if (missingProfileData)
{
JITDUMP("Profile data could not be locally repaired. Data was missing.\n");
}

if (!repairWasComplete)
{
JITDUMP("Profile data could not be locally repaired. Data %s inconsistent.\n",
m_compiler->fgPgoConsistent ? "is now" : "was already");

if (m_compiler->fgPgoConsistent)
{
m_compiler->Metrics.ProfileInconsistentInlinerBranchFold++;
m_compiler->fgPgoConsistent = false;
}
}
}
// Update profile, make it consistent if possible.
//
m_compiler->fgRepairProfileCondToUncond(block, retainedEdge, removedEdge,
&m_compiler->Metrics.ProfileInconsistentInlinerBranchFold);
}
}
else
Expand Down
112 changes: 112 additions & 0 deletions src/coreclr/jit/fgprofile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5063,3 +5063,115 @@ bool Compiler::fgDebugCheckOutgoingProfileData(BasicBlock* block, ProfileChecks
}

#endif // DEBUG

//------------------------------------------------------------------------
// fgRepairProfileCondToUncond: attempt to repair profile after modifying
// a conditinal branch to an unconditional branch.
//
// Arguments:
// block - block that was just altered
// retainedEdge - flow edge that remains
// removedEdge - flow edge that was removed
// metric - [in/out, optional] metric to update if profile becomes inconsistent
//
void Compiler::fgRepairProfileCondToUncond(BasicBlock* block,
FlowEdge* retainedEdge,
FlowEdge* removedEdge,
int* metric)
{
assert(block->KindIs(BBJ_ALWAYS));
assert(block->GetTargetEdge() == retainedEdge);

// If block does not have profile data, there's nothing to do.
//
if (!block->hasProfileWeight())
{
return;
}

// If the removed edge was not carrying away any profile, there's nothing to do.
//
weight_t const weight = removedEdge->getLikelyWeight();

if (weight == 0)
{
return;
}

// If the branch was degenerate, there is nothing to do
//
if (retainedEdge == removedEdge)
{
return;
}

// This flow graph change will affect profile transitively, so in general
// the profile will become inconsistent.
//
bool repairWasComplete = false;
bool missingProfileData = false;

// Target block weight will increase.
//
BasicBlock* const target = block->GetTarget();

if (target->hasProfileWeight())
{
target->setBBProfileWeight(target->bbWeight + weight);
}
else
{
missingProfileData = true;
}

// Alternate weight will decrease
//
BasicBlock* const alternate = removedEdge->getDestinationBlock();

if (alternate->hasProfileWeight())
{
weight_t const alternateNewWeight = alternate->bbWeight - weight;

// If profile weights are consistent, expect at worst a slight underflow.
//
if (fgPgoConsistent && (alternateNewWeight < 0))
{
assert(fgProfileWeightsEqual(alternateNewWeight, 0));
}
alternate->setBBProfileWeight(max(0.0, alternateNewWeight));
}
else
{
missingProfileData = true;
}

// Check for the special case where the block's postdominator
// is target's target (simple if/then/else/join).
//
// TODO: try a bit harder to find a postdominator, if it's "nearby"
//
if (!missingProfileData && target->KindIs(BBJ_ALWAYS))
{
repairWasComplete = alternate->KindIs(BBJ_ALWAYS) && (alternate->GetTarget() == target->GetTarget());
}

if (missingProfileData)
{
JITDUMP("Profile data could not be locally repaired. Data was missing.\n");
}

if (!repairWasComplete)
{
JITDUMP("Profile data could not be locally repaired. Data %s inconsistent.\n",
fgPgoConsistent ? "is now" : "was already");

if (fgPgoConsistent)
{
if (metric != nullptr)
{
*metric++;
}
fgPgoConsistent = false;
}
}
}
60 changes: 2 additions & 58 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7613,64 +7613,8 @@ void Compiler::impImportBlockCode(BasicBlock* block)
fgRemoveRefPred(removedEdge);
block->SetKindAndTargetEdge(BBJ_ALWAYS, retainedEdge);
Metrics.ImporterBranchFold++;

// If we removed an edge carrying profile, try to do a local repair.
//
if (block->hasProfileWeight())
{
bool repairWasComplete = true;
weight_t const weight = removedEdge->getLikelyWeight();

if (weight > 0)
{
// Target block weight will increase.
//
BasicBlock* const target = block->GetTarget();
assert(target->hasProfileWeight());
target->setBBProfileWeight(target->bbWeight + weight);

// Alternate weight will decrease
//
BasicBlock* const alternate = removedEdge->getDestinationBlock();
assert(alternate->hasProfileWeight());
weight_t const alternateNewWeight = alternate->bbWeight - weight;

// If profile weights are consistent, expect at worst a slight underflow.
//
if (fgPgoConsistent && (alternateNewWeight < 0))
{
assert(fgProfileWeightsEqual(alternateNewWeight, 0));
}
alternate->setBBProfileWeight(max(0.0, alternateNewWeight));

// This will affect profile transitively, so in general
// the profile will become inconsistent.
//
repairWasComplete = false;

// But we can check for the special case where the
// block's postdominator is target's target (simple
// if/then/else/join).
//
if (target->KindIs(BBJ_ALWAYS))
{
repairWasComplete = alternate->KindIs(BBJ_ALWAYS) &&
(alternate->GetTarget() == target->GetTarget());
}
}

if (!repairWasComplete)
{
JITDUMP("Profile data could not be locally repaired. Data %s inconsistent.\n",
fgPgoConsistent ? "is now" : "was already");

if (fgPgoConsistent)
{
Metrics.ProfileInconsistentImporterBranchFold++;
fgPgoConsistent = false;
}
}
}
fgRepairProfileCondToUncond(block, retainedEdge, removedEdge,
&Metrics.ProfileInconsistentImporterBranchFold);
}

if (!op1->OperIs(GT_CNS_INT))
Expand Down
9 changes: 1 addition & 8 deletions src/coreclr/jit/objectalloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,14 +669,7 @@ unsigned int ObjectAllocator::MorphAllocObjNodeIntoStackAlloc(
BasicBlock* removedBlock = removedEdge->getDestinationBlock();
comp->fgRemoveRefPred(removedEdge);
predBlock->SetKindAndTargetEdge(BBJ_ALWAYS, keptEdge);

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;
}
comp->fgRepairProfileCondToUncond(predBlock, keptEdge, removedEdge);

// Just lop off the JTRUE, the rest can clean up later
// (eg may have side effects)
Expand Down
Loading