Skip to content

Commit

Permalink
JIT: Check for profile consistency throughout JIT backend (#111684)
Browse files Browse the repository at this point in the history 
Part of #107749.
  • Loading branch information
@amanasifkhalid
amanasifkhalid authored Jan 30, 2025
1 parent f652094 commit aef8f42
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 35 deletions.
10 changes: 0 additions & 10 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4872,11 +4872,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
DoPhase(this, PHASE_COMPUTE_DOMINATORS, &Compiler::fgComputeDominators);
}

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

#ifdef DEBUG
fgDebugCheckLinks();
#endif
Expand Down Expand Up @@ -5157,11 +5152,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
DoPhase(this, PHASE_SWITCH_RECOGNITION, &Compiler::optSwitchRecognition);
}

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

#ifdef DEBUG
// Stash the current estimate of the function's size if necessary.
if (verbose && opts.OptimizationEnabled())
Expand Down
40 changes: 22 additions & 18 deletions src/coreclr/jit/helperexpansion.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ bool Compiler::fgExpandThreadLocalAccessForCallNativeAOT(BasicBlock** pBlock, St
fastPathBb->inheritWeight(prevBb);

// fallback will just execute first time
fallbackBb->bbSetRunRarely();
fallbackBb->inheritWeightPercentage(tlsRootNullCondBB, 0);

fgRedirectTargetEdge(prevBb, tlsRootNullCondBB);

Expand Down Expand Up @@ -1180,7 +1180,7 @@ bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock** pBlock, Statement*
fastPathBb->inheritWeight(prevBb);

// fallback will just execute first time
fallbackBb->bbSetRunRarely();
fallbackBb->inheritWeightPercentage(prevBb, 0);

// All blocks are expected to be in the same EH region
assert(BasicBlock::sameEHRegion(prevBb, block));
Expand Down Expand Up @@ -1545,7 +1545,7 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, G

block->inheritWeight(prevBb);
isInitedBb->inheritWeight(prevBb);
helperCallBb->bbSetRunRarely();
helperCallBb->inheritWeightPercentage(isInitedBb, 0);

// All blocks are expected to be in the same EH region
assert(BasicBlock::sameEHRegion(prevBb, block));
Expand Down Expand Up @@ -1847,6 +1847,7 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock,
//
// Redirect prevBb to lengthCheckBb
fgRedirectTargetEdge(prevBb, lengthCheckBb);
lengthCheckBb->inheritWeight(prevBb);
assert(prevBb->JumpsToNext());

{
Expand All @@ -1859,6 +1860,11 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock,
// review: we assume length check always succeeds??
trueEdge->setLikelihood(1.0);
falseEdge->setLikelihood(0.0);

if (lengthCheckBb->hasProfileWeight())
{
fastpathBb->setBBProfileWeight(falseEdge->getLikelyWeight());
}
}

{
Expand All @@ -1869,10 +1875,8 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock,
}

//
// Re-distribute weights
// Ensure all flow out of prevBb converges into block
//
lengthCheckBb->inheritWeight(prevBb);
fastpathBb->inheritWeight(lengthCheckBb);
block->inheritWeight(prevBb);

// All blocks are expected to be in the same EH region
Expand Down Expand Up @@ -2551,11 +2555,18 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt,
trueEdge->setLikelihood(nullcheckTrueLikelihood);
}

// Set nullcheckBb's weight here, so we can propagate it to its successors below
nullcheckBb->inheritWeight(firstBb);

if (typeCheckNotNeeded)
{
FlowEdge* const falseEdge = fgAddRefPred(fallbackBb, nullcheckBb);
nullcheckBb->SetFalseEdge(falseEdge);
falseEdge->setLikelihood(nullcheckFalseLikelihood);
fallbackBb->inheritWeight(nullcheckBb);
fallbackBb->scaleBBWeight(nullcheckFalseLikelihood);
lastBb->inheritWeight(nullcheckBb);
lastBb->scaleBBWeight(nullcheckTrueLikelihood);

typeCheckSucceedBb = nullptr;
}
Expand Down Expand Up @@ -2631,7 +2642,6 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt,
// The same goes for inherited weights -- the block where we test for B will have
// the weight of A times the likelihood that A's test fails, etc.
//
nullcheckBb->inheritWeight(firstBb);
weight_t sumOfPreviousLikelihood = 0;
for (int candidateId = 0; candidateId < numOfCandidates; candidateId++)
{
Expand Down Expand Up @@ -2666,28 +2676,22 @@ bool Compiler::fgLateCastExpansionForCall(BasicBlock** pBlock, Statement* stmt,
sumOfPreviousLikelihood += likelihood;
}

if (fallbackBb->KindIs(BBJ_THROW))
{
fallbackBb->bbSetRunRarely();
}
else
fallbackBb->inheritWeight(lastTypeCheckBb);
fallbackBb->scaleBBWeight(lastTypeCheckBb->GetFalseEdge()->getLikelihood());

if (fallbackBb->KindIs(BBJ_ALWAYS))
{
assert(fallbackBb->KindIs(BBJ_ALWAYS));
FlowEdge* const newEdge = fgAddRefPred(lastBb, fallbackBb);
fallbackBb->SetTargetEdge(newEdge);
fallbackBb->inheritWeight(lastTypeCheckBb);
weight_t lastTypeCheckFailedLikelihood = lastTypeCheckBb->GetFalseEdge()->getLikelihood();
fallbackBb->scaleBBWeight(lastTypeCheckFailedLikelihood);
}

if (!typeCheckNotNeeded)
{
typeCheckSucceedBb->inheritWeight(typeChecksBbs[0]);
typeCheckSucceedBb->scaleBBWeight(sumOfPreviousLikelihood);
lastBb->inheritWeight(firstBb);
}

lastBb->inheritWeight(firstBb);

//
// Validate EH regions
//
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/jiteh.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2660,7 +2660,7 @@ bool Compiler::fgCreateFiltersForGenericExceptions()
filterBb->bbCodeOffs = handlerBb->bbCodeOffs;
filterBb->bbHndIndex = handlerBb->bbHndIndex;
filterBb->bbTryIndex = handlerBb->bbTryIndex;
filterBb->bbSetRunRarely();
filterBb->inheritWeightPercentage(handlerBb, 0);
filterBb->SetFlags(BBF_INTERNAL | BBF_DONT_REMOVE);

handlerBb->bbCatchTyp = BBCT_FILTER_HANDLER;
Expand Down
65 changes: 59 additions & 6 deletions src/coreclr/jit/lower.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1071,11 +1071,11 @@ GenTree* Lowering::LowerSwitch(GenTree* node)
for (unsigned i = 0; i < jumpCnt - 1; ++i)
{
assert(currentBlock != nullptr);
BasicBlock* const targetBlock = jumpTab[i]->getDestinationBlock();

// Remove the switch from the predecessor list of this case target's block.
// We'll add the proper new predecessor edge later.
FlowEdge* const oldEdge = jumpTab[i];
FlowEdge* const oldEdge = jumpTab[i];
BasicBlock* const targetBlock = oldEdge->getDestinationBlock();

// Compute the likelihood that this test is successful.
// Divide by number of cases still sharing this edge (reduces likelihood)
Expand Down Expand Up @@ -1136,8 +1136,9 @@ GenTree* Lowering::LowerSwitch(GenTree* node)
{
BasicBlock* const newBlock = comp->fgNewBBafter(BBJ_ALWAYS, currentBlock, true);
FlowEdge* const newEdge = comp->fgAddRefPred(newBlock, currentBlock);
currentBlock = newBlock;
currentBBRange = &LIR::AsRange(currentBlock);
newBlock->inheritWeight(currentBlock);
currentBlock = newBlock;
currentBBRange = &LIR::AsRange(currentBlock);
afterDefaultCondBlock->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge);
}

Expand Down Expand Up @@ -1212,6 +1213,25 @@ GenTree* Lowering::LowerSwitch(GenTree* node)
currentBlock->RemoveFlags(BBF_DONT_REMOVE);
comp->fgRemoveBlock(currentBlock, /* unreachable */ false); // It's an empty block.
}

// Update flow into switch targets
if (afterDefaultCondBlock->hasProfileWeight())
{
bool profileInconsistent = false;
for (unsigned i = 0; i < jumpCnt - 1; i++)
{
BasicBlock* const targetBlock = jumpTab[i]->getDestinationBlock();
targetBlock->setBBProfileWeight(targetBlock->computeIncomingWeight());
profileInconsistent |= (targetBlock->NumSucc() > 0);
}

if (profileInconsistent)
{
JITDUMP("Switch lowering: Flow out of " FMT_BB " needs to be propagated. Data %s inconsistent.\n",
afterDefaultCondBlock->bbNum, comp->fgPgoConsistent ? "is now" : "was already");
comp->fgPgoConsistent = false;
}
}
}
else
{
Expand Down Expand Up @@ -1265,11 +1285,28 @@ GenTree* Lowering::LowerSwitch(GenTree* node)
JITDUMP("Zero weight switch block " FMT_BB ", distributing likelihoods equally per case\n",
afterDefaultCondBlock->bbNum);
// jumpCnt-1 here because we peeled the default after copying this value.
weight_t const newLikelihood = 1.0 / (jumpCnt - 1);
weight_t const newLikelihood = 1.0 / (jumpCnt - 1);
bool profileInconsistent = false;
for (unsigned i = 0; i < successors.numDistinctSuccs; i++)
{
FlowEdge* const edge = successors.nonDuplicates[i];
FlowEdge* const edge = successors.nonDuplicates[i];
weight_t const oldEdgeWeight = edge->getLikelyWeight();
edge->setLikelihood(newLikelihood * edge->getDupCount());
weight_t const newEdgeWeight = edge->getLikelyWeight();

if (afterDefaultCondBlock->hasProfileWeight())
{
BasicBlock* const targetBlock = edge->getDestinationBlock();
targetBlock->increaseBBProfileWeight(newEdgeWeight - oldEdgeWeight);
profileInconsistent |= (targetBlock->NumSucc() > 0);
}
}

if (profileInconsistent)
{
JITDUMP("Switch lowering: Flow out of " FMT_BB " needs to be propagated. Data %s inconsistent.\n",
afterDefaultCondBlock->bbNum, comp->fgPgoConsistent ? "is now" : "was already");
comp->fgPgoConsistent = false;
}
}
else
Expand Down Expand Up @@ -1452,6 +1489,22 @@ bool Lowering::TryLowerSwitchToBitTest(FlowEdge* jumpTable[],

bbSwitch->SetCond(case1Edge, case0Edge);

//
// Update profile
//
if (bbSwitch->hasProfileWeight())
{
bbCase0->setBBProfileWeight(bbCase0->computeIncomingWeight());
bbCase1->setBBProfileWeight(bbCase1->computeIncomingWeight());

if ((bbCase0->NumSucc() > 0) || (bbCase1->NumSucc() > 0))
{
JITDUMP("TryLowerSwitchToBitTest: Flow out of " FMT_BB " needs to be propagated. Data %s inconsistent.\n",
bbSwitch->bbNum, comp->fgPgoConsistent ? "is now" : "was already");
comp->fgPgoConsistent = false;
}
}

var_types bitTableType = (bitCount <= (genTypeSize(TYP_INT) * 8)) ? TYP_INT : TYP_LONG;
GenTree* bitTableIcon = comp->gtNewIconNode(bitTable, bitTableType);

Expand Down
11 changes: 11 additions & 0 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2240,6 +2240,8 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block)
//
bNewCond->inheritWeight(block);

const weight_t totalWeight = bTest->bbWeight;

if (haveProfileWeights)
{
bTest->decreaseBBProfileWeight(block->bbWeight);
Expand Down Expand Up @@ -2300,6 +2302,15 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block)
}
}

const weight_t loopWeight = bTest->bbWeight;
const weight_t nonLoopWeight = bNewCond->bbWeight;
if (haveProfileWeights && !fgProfileWeightsConsistent(totalWeight, loopWeight + nonLoopWeight))
{
JITDUMP("Redirecting flow from " FMT_BB " to " FMT_BB " introduced inconsistency. Data %s inconsistent.\n",
bTest->bbNum, bNewCond->bbNum, fgPgoConsistent ? "is now" : "was already");
fgPgoConsistent = false;
}

#ifdef DEBUG
if (verbose)
{
Expand Down

0 comments on commit aef8f42

Please sign in to comment.