Skip to content

Fix PGO consistency asserts with Then only QMarks with likelyhood in AO #129594

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

Open
MichalPetryka wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −34
Open

Fix PGO consistency asserts with Then only QMarks with likelyhood in AO #129594

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
55 changes: 21 additions & 34 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14807,17 +14807,31 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt, bool onlyEa
}
#endif // DEBUG

GenTreeColon* colon = qmark->gtGetOp2()->AsColon();
bool hasTrueExpr = !colon->ThenNode()->OperIs(GT_NOP);
bool hasFalseExpr = !colon->ElseNode()->OperIs(GT_NOP);
assert(hasTrueExpr || hasFalseExpr); // We expect to have at least one arm of the qmark!

// canonicalize to else
if (hasTrueExpr && !hasFalseExpr)
{
qmark->gtOp1 = gtReverseCond(qmark->gtGetOp1());
qmark->SetThenNodeLikelihood(qmark->ElseNodeLikelihood());

GenTree* op2 = colon->gtOp2;
colon->gtOp2 = colon->gtOp1;
colon->gtOp1 = op2;
hasTrueExpr = false;
hasFalseExpr = true;
}
assert(hasFalseExpr); // we should always have false now

@AndyAyersMS AndyAyersMS Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems a bit odd to canonicalize to else, is this just to avoid re-reversing the cond?

@MichalPetryka MichalPetryka Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well the code was already reversing stuff which I assume was the part causing issues and doing it properly here once lets us not worry about having to handle only-then and only-else separately.

@AndyAyersMS AndyAyersMS Jun 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I get that, but the existing code that reverses has a TODO saying reversing isn't needed and now you've introduced more reversing.

So I wonder if there is a reversing-free approach that would apply nicely to both.


// Retrieve the operands.
GenTree* trueExpr = colon->ThenNode();
GenTree* falseExpr = colon->ElseNode();
GenTree* condExpr = qmark->gtGetOp1();
GenTree* trueExpr = qmark->gtGetOp2()->AsColon()->ThenNode();
GenTree* falseExpr = qmark->gtGetOp2()->AsColon()->ElseNode();

assert(!varTypeIsFloating(condExpr->TypeGet()));

bool hasTrueExpr = !trueExpr->OperIs(GT_NOP);
bool hasFalseExpr = !falseExpr->OperIs(GT_NOP);
assert(hasTrueExpr || hasFalseExpr); // We expect to have at least one arm of the qmark!

// Create remainder, cond and "else" blocks. After this, the blocks are in this order:
// block ... condBlock ... elseBlock ... remainderBlock
//
Expand Down Expand Up @@ -14895,33 +14909,6 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt, bool onlyEa
thenEdge->setLikelihood(thenLikelihood / 100.0);
elseEdge->setLikelihood(elseLikelihood / 100.0);
}
else if (hasTrueExpr)
{
// false
// S0 -->-- ~C -->-- T -->-- S1
// | |
// +-->-------------+
// bbj_cond(true)
//
// TODO: Remove unnecessary condition reversal
qmark->gtOp1 = condExpr = gtReverseCond(condExpr);

const unsigned thenLikelihood = qmark->ThenNodeLikelihood();
const unsigned elseLikelihood = qmark->ElseNodeLikelihood();

assert(condBlock->TargetIs(elseBlock));
FlowEdge* const thenEdge = fgAddRefPred(remainderBlock, condBlock);
FlowEdge* const elseEdge = condBlock->GetTargetEdge();
condBlock->SetCond(thenEdge, elseEdge);

// Since we have no false expr, use the one we'd already created.
thenBlock = elseBlock;
elseBlock = nullptr;

thenBlock->inheritWeightPercentage(condBlock, thenLikelihood);
thenEdge->setLikelihood(thenLikelihood / 100.0);
elseEdge->setLikelihood(elseLikelihood / 100.0);
}
else if (hasFalseExpr)
{
// false
Expand Down
Loading