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

Allow gtFoldExprSpecial to handle side effects #103382

Merged
merged 2 commits into from
Jun 13, 2024
Merged

Allow gtFoldExprSpecial to handle side effects #103382

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b67f027
Allow gtFoldExprSpecial to handle side effects
tannergooding Jun 12, 2024
ceb7035
Ensure that return expressions are properly treated as having side ef…
tannergooding Jun 13, 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
105 changes: 65 additions & 40 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14327,15 +14327,12 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)

val = cons->AsIntConCommon()->IconValue();

// Transforms that would drop op cannot be performed if op has side effects
bool opHasSideEffects = (op->gtFlags & GTF_SIDE_EFFECT) != 0;

// Helper function that creates a new IntCon node and morphs it, if required
auto NewMorphedIntConNode = [&](int value) -> GenTreeIntCon* {
GenTreeIntCon* icon = gtNewIconNode(value);
if (fgGlobalMorph)
{
fgMorphTreeDone(icon);
INDEBUG(icon->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
}
return icon;
};
Expand Down Expand Up @@ -14370,43 +14367,52 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
switch (oper)
{
case GT_LE:
if (tree->IsUnsigned() && (val == 0) && (op1 == cons) && !opHasSideEffects)
{
if (tree->IsUnsigned() && (val == 0) && (op1 == cons))
{
// unsigned (0 <= x) is always true
op = NewMorphedIntConNode(1);
op = gtWrapWithSideEffects(NewMorphedIntConNode(1), op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
break;
}

case GT_GE:
if (tree->IsUnsigned() && (val == 0) && (op2 == cons) && !opHasSideEffects)
{
if (tree->IsUnsigned() && (val == 0) && (op2 == cons))
{
// unsigned (x >= 0) is always true
op = NewMorphedIntConNode(1);
op = gtWrapWithSideEffects(NewMorphedIntConNode(1), op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
break;
}

case GT_LT:
if (tree->IsUnsigned() && (val == 0) && (op2 == cons) && !opHasSideEffects)
{
if (tree->IsUnsigned() && (val == 0) && (op2 == cons))
{
// unsigned (x < 0) is always false
op = NewMorphedIntConNode(0);
op = gtWrapWithSideEffects(NewMorphedIntConNode(0), op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
break;
}

case GT_GT:
if (tree->IsUnsigned() && (val == 0) && (op1 == cons) && !opHasSideEffects)
{
if (tree->IsUnsigned() && (val == 0) && (op1 == cons))
{
// unsigned (0 > x) is always false
op = NewMorphedIntConNode(0);
op = gtWrapWithSideEffects(NewMorphedIntConNode(0), op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
}
FALLTHROUGH;

case GT_EQ:
case GT_NE:

{
// Optimize boxed value classes; these are always false. This IL is
// generated when a generic value is tested against null:
// <T> ... foo(T x) { ... if ((object)x == null) ...
Expand Down Expand Up @@ -14468,57 +14474,59 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
{
return gtFoldBoxNullable(tree);
}

break;
}

case GT_ADD:
{
if (val == 0)
{
goto DONE_FOLD;
}
break;
}

case GT_MUL:
{
if (val == 1)
{
goto DONE_FOLD;
}
else if (val == 0)
{
/* Multiply by zero - return the 'zero' node, but not if side effects */
if (!opHasSideEffects)
{
op = cons;
goto DONE_FOLD;
}
// Multiply by zero - return the 'zero' node
op = gtWrapWithSideEffects(cons, op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
break;
}

case GT_DIV:
case GT_UDIV:
{
if ((op2 == cons) && (val == 1) && !op1->OperIsConst())
{
goto DONE_FOLD;
}
break;
}

case GT_SUB:
{
if ((op2 == cons) && (val == 0) && !op1->OperIsConst())
{
goto DONE_FOLD;
}
break;
}

case GT_AND:
{
if (val == 0)
{
/* AND with zero - return the 'zero' node, but not if side effects */

if (!opHasSideEffects)
{
op = cons;
goto DONE_FOLD;
}
// AND with zero - return the 'zero' node
op = gtWrapWithSideEffects(cons, op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
else if (val == 0xFF)
{
Expand Down Expand Up @@ -14551,8 +14559,10 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
}
}
break;
}

case GT_OR:
{
if (val == 0)
{
goto DONE_FOLD;
Expand All @@ -14563,34 +14573,33 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)

assert(val == 1);

/* OR with one - return the 'one' node, but not if side effects */

if (!opHasSideEffects)
{
op = cons;
goto DONE_FOLD;
}
// OR with one - return the 'one' node
op = gtWrapWithSideEffects(cons, op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
break;
}

case GT_LSH:
case GT_RSH:
case GT_RSZ:
case GT_ROL:
case GT_ROR:
{
if (val == 0)
{
if (op2 == cons)
{
goto DONE_FOLD;
}
else if (!opHasSideEffects)
else
{
op = cons;
op = gtWrapWithSideEffects(cons, op, GTF_ALL_EFFECT);
goto DONE_FOLD;
}
}
break;
}

case GT_QMARK:
{
Expand All @@ -14613,9 +14622,8 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
{
fgWalkTreePre(&op, gtClearColonCond);
}
}

goto DONE_FOLD;
}

default:
break;
Expand All @@ -14632,6 +14640,13 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
JITDUMP("Transformed into:\n");
DISPTREE(op);

if (fgGlobalMorph)
{
// We can sometimes produce a comma over the constant if the original op
// had a side effect, so just ensure we set the flag (which will be already
// set for the operands otherwise).
INDEBUG(op->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
}
return op;
}

Expand Down Expand Up @@ -16903,9 +16918,19 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags)
// Are there only GTF_CALL side effects remaining? (and no other side effect kinds)
if (flags & GTF_CALL)
{
if (tree->OperGet() == GT_CALL)
GenTree* potentialCall = tree;

if (potentialCall->OperIs(GT_RET_EXPR))
{
// We need to preserve return expressions where the underlying call
// has side effects. Otherwise early folding can result in us dropping
// the call.
potentialCall = potentialCall->AsRetExpr()->gtInlineCandidate;
}
Comment on lines +16921 to +16929
Copy link
Member Author

@tannergooding tannergooding Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To comment on this fix a bit more...

gtNodeHasSideEffects is trying to see if it really has side effects or is just marked such because some child node has side effects

It's mainly used as part of gtExtractSideEffList which is trying to build a list of all the real side effects
throwing away anything that isn't itself directly side effecting
i.e. since ADD(call, cns) would be marked as CALL due to propagating side effect flags up through the parent and therefore we want to just save the call in the produced comma, not the add or cns

so it was just missing the fact that GT_RET_EXPR is technically a call itself, just a placeholder until the latter IR transforms take place
which is relevant since the inliner, if it fails, actually removes the block containing the call assuming that GT_RET_EXPR will replace itself with the call
but, we'd have dropped the GT_RET_EXPR as non side effecting if it happened too early (such as happens for gtFoldExprSpecial since it happens as part of normal IL importation and evaluation)


if (potentialCall->OperIs(GT_CALL))
{
GenTreeCall* const call = tree->AsCall();
GenTreeCall* const call = potentialCall->AsCall();
const bool ignoreExceptions = (flags & GTF_EXCEPT) == 0;
const bool ignoreCctors = (flags & GTF_IS_IN_CSE) != 0; // We can CSE helpers that run cctors.
if (!call->HasSideEffects(this, ignoreExceptions, ignoreCctors))
Expand Down
Loading