Skip to content

Commit

Permalink
JIT: extract BBJ_COND to BBJ_ALWAYS profile repair as utility (dotnet…
Browse files Browse the repository at this point in the history 
…#110494)

JIT: extract BBJ_COND to BBJ_ALWAYS profile repair as utility

and call it from a few places.

Co-authored-by: Aman Khalid <[email protected]>
  • Loading branch information
@AndyAyersMS @amanasifkhalid @hez2010
2 people authored and hez2010 committed Dec 14, 2024
1 parent 511b2e0 commit 810bad5
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 149 deletions.
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
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
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
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 /* = nullptr */)
{
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.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.0))
{
assert(fgProfileWeightsEqual(alternateNewWeight, 0.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
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
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

0 comments on commit 810bad5

Please sign in to comment.