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

Small clean up in assertionprop for relops #109987

Merged
merged 3 commits into from
Nov 21, 2024
+27 −72
Merged

Small clean up in assertionprop for relops #109987

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd9f1f1
Small clean up in assertionprop for relops
EgorBo Nov 19, 2024
e0aeabb
fix typo
EgorBo Nov 19, 2024
82c458d
Address feedback
EgorBo Nov 21, 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
99 changes: 27 additions & 72 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3921,6 +3921,18 @@ void Compiler::optAssertionProp_RangeProperties(ASSERT_VALARG_TP assertions,
continue;
}

// Same for Length, example:
//
// array[idx] = 42;
// array.Length is known to be non-negative and non-zero here
//
if (curAssertion->IsBoundsCheckNoThrow() && (curAssertion->op1.bnd.vnLen == treeVN))
{
*isKnownNonNegative = true;
*isKnownNonZero = true;
return; // both properties are known, no need to check other assertions
}

// First, analyze possible X ==/!= CNS assertions.
if (curAssertion->IsConstantInt32Assertion() && (curAssertion->op1.vn == treeVN))
{
Expand Down Expand Up @@ -4278,23 +4290,9 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen
}
#endif

// Bail out if tree is not side effect free.
if ((tree->gtFlags & GTF_SIDE_EFFECT) != 0)
{
JITDUMP("sorry, blocked by side effects\n");
return nullptr;
}

if (curAssertion->assertionKind == OAK_EQUAL)
{
tree->BashToConst(0);
}
else
{
tree->BashToConst(1);
}

newTree = fgMorphTree(tree);
newTree = curAssertion->assertionKind == OAK_EQUAL ? gtNewIconNode(0) : gtNewIconNode(1);
newTree = gtWrapWithSideEffects(newTree, tree, GTF_ALL_EFFECT);
newTree = fgMorphTree(newTree);
DISPTREE(newTree);
return optAssertionProp_Update(newTree, tree, stmt);
}
Expand Down Expand Up @@ -6219,33 +6217,16 @@ struct VNAssertionPropVisitorInfo

//------------------------------------------------------------------------------
// optVNConstantPropOnJTrue
// Constant propagate on the JTrue node by extracting side effects and moving
// them into their own statements. The relop node is then modified to yield
// true or false, so the branch can be folded.
// Constant propagate on the JTrue node.
//
// Arguments:
// block - The block that contains the JTrue.
// test - The JTrue node whose relop evaluates to 0 or non-zero value.
//
// Return Value:
// The jmpTrue tree node that has relop of the form "0 =/!= 0".
// If "tree" evaluates to "true" relop is "0 == 0". Else relop is "0 != 0".
//
// Description:
// Special treatment for JTRUE nodes' constant propagation. This is because
// for JTRUE(1) or JTRUE(0), if there are side effects they need to be put
// in separate statements. This is to prevent relop's constant
// propagation from doing a simple minded conversion from
// (1) STMT(JTRUE(RELOP(COMMA(sideEffect, OP1), OP2)), S.T. op1 =/!= op2 to
// (2) STMT(JTRUE(COMMA(sideEffect, 1/0)).
//
// fgFoldConditional doesn't fold (2), a side-effecting JTRUE's op1. So, let us,
// here, convert (1) as two statements: STMT(sideEffect), STMT(JTRUE(1/0)),
// so that the JTRUE will get folded by fgFoldConditional.
//
// Note: fgFoldConditional is called from other places as well, which may be
// sensitive to adding new statements. Hence the change is not made directly
// into fgFoldConditional.
// nullptr if no constant propagation is done, else the modified JTrue node
// containing "0==0" or "0!=0" relop node
// (where op1 is wrapped with side effects if any).
//
GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test)
{
Expand All @@ -6266,49 +6247,23 @@ GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test)

// We want to use the Normal ValueNumber when checking for constants.
ValueNum vnCns = vnStore->VNConservativeNormalValue(relop->gtVNPair);
ValueNum vnLib = vnStore->VNLiberalNormalValue(relop->gtVNPair);
if (!vnStore->IsVNConstant(vnCns))
{
return nullptr;
}

// Prepare the tree for replacement so any side effects can be extracted.
GenTree* sideEffList = nullptr;
gtExtractSideEffList(relop, &sideEffList, GTF_SIDE_EFFECT, true);

// Transform the relop's operands to be both zeroes.
ValueNum vnZero = vnStore->VNZeroForType(TYP_INT);
relop->AsOp()->gtOp1 = gtNewIconNode(0);
relop->AsOp()->gtOp1->gtVNPair = ValueNumPair(vnZero, vnZero);
relop->AsOp()->gtOp2 = gtNewIconNode(0);
relop->AsOp()->gtOp2->gtVNPair = ValueNumPair(vnZero, vnZero);

// Update the oper and restore the value numbers.
bool evalsToTrue = (vnStore->CoercedConstantValue<INT64>(vnCns) != 0);
relop->SetOper(evalsToTrue ? GT_EQ : GT_NE);
relop->gtVNPair = ValueNumPair(vnLib, vnCns);

// Insert side effects back after they were removed from the JTrue stmt.
// It is important not to allow duplicates exist in the IR, that why we delete
// these side effects from the JTrue stmt before insert them back here.
while (sideEffList != nullptr)
GenTree* sideEffects = gtWrapWithSideEffects(gtNewNothingNode(), relop);
if (!sideEffects->IsNothingNode())
{
Statement* newStmt;
if (sideEffList->OperGet() == GT_COMMA)
{
newStmt = fgNewStmtNearEnd(block, sideEffList->gtGetOp1());
sideEffList = sideEffList->gtGetOp2();
}
else
{
newStmt = fgNewStmtNearEnd(block, sideEffList);
sideEffList = nullptr;
}
// fgMorphBlockStmt could potentially affect stmts after the current one,
// for example when it decides to fgRemoveRestOfBlock.
// Insert side effects before the JTRUE stmt.
Statement* newStmt = fgNewStmtNearEnd(block, sideEffects);
fgMorphBlockStmt(block, newStmt DEBUGARG(__FUNCTION__));
}

// Let's maintain the invariant that JTRUE's operand is always a relop.
// and if we have side effects, we wrap one of the operands with them, not the relop.
const bool evalsToTrue = (vnStore->CoercedConstantValue<INT64>(vnCns) != 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

note: we already produce a lot of JTRUE(1) nodes when opts are enabled (they're not allowed in tier0 since lower might not expect it)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we do. At this point in the JIT we have the invariant that the child of a JTRUE is always a relop.
Is this change making assertion prop produce JTRUE(non-relop) that other phases will see? That's changing IR invariants. The other opts phases might not handle that well (so specifically when JitOptRepeat is enabled). Lowering should handle it fine both with and without optimizations enabled as I removed the invariant from the backend in #82766.

Some phases (like morph when optimizing) temporarily introduce the pattern, but they also fold it away before the phase ends.

I would be fine with removing the invariant, but then the change should be explicit about the fact that it's changing the invariants, and I think we should do it more globally (e.g. validate that it's fine for all the opts phases).

Copy link
Member Author

Choose a reason for hiding this comment

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

@jakobbotsch this code feeds morph so it is always folded, but ok, I've relaxed it back to 0==0. Looks like there is a room for improvement here, e.g. we refuse to fold branches where one of the constant has side-effects via COMMA

Copy link
Member

Choose a reason for hiding this comment

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

If this is folded within assertion prop then the previous implementation was ok as well, but this one looks fine with me too.

test->AsOp()->gtOp1 = gtNewOperNode(evalsToTrue ? GT_EQ : GT_NE, relop->TypeGet(), gtNewFalse(), gtNewFalse());
Copy link
Member

Choose a reason for hiding this comment

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

Does it matter that this doesn't VN the new nodes? The old logic did.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it matter that this doesn't VN the new nodes? The old logic did.

My understanding that it always invokes morph right after this and morph folds JTRUE(0==0) so it's not necessary. It also has no impact on diffs

return test;
}

Expand Down
Loading