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.

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

JIT: Fix gtNodeHasSideEffects checking call arguments #106185

Merged
merged 3 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 1 addition & 7 deletions src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6254,13 +6254,7 @@ GenTree* Compiler::optExtractSideEffListFromConst(GenTree* tree)
// Do a sanity check to ensure persistent side effects aren't discarded and
// tell gtExtractSideEffList to ignore the root of the tree.
// We are relying here on an invariant that VN will only fold non-throwing expressions.
const bool ignoreExceptions = true;
const bool ignoreCctors = false;
// We have to check "AsCall()->HasSideEffects()" here separately because "gtNodeHasSideEffects"
// also checks for side effects that arguments introduce (incosistently so, it otherwise only
// checks for the side effects the node itself has). TODO-Cleanup: change it to not do that?
assert(!gtNodeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS) ||
(tree->IsCall() && !tree->AsCall()->HasSideEffects(this, ignoreExceptions, ignoreCctors)));
assert(!gtNodeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS));

// Exception side effects may be ignored because the root is known to be a constant
// (e.g. VN may evaluate a DIV/MOD node to a constant and the node may still
Expand Down
53 changes: 20 additions & 33 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16828,31 +16828,7 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags, bool igno
{
GenTreeCall* const call = potentialCall->AsCall();
const bool ignoreExceptions = (flags & GTF_EXCEPT) == 0;
if (!call->HasSideEffects(this, ignoreExceptions, ignoreCctors))
{
// If this call is otherwise side effect free, check its arguments.
for (CallArg& arg : call->gtArgs.Args())
{
// I'm a little worried that args that assign to temps that are late args will look like
// side effects...but better to be conservative for now.
if ((arg.GetEarlyNode() != nullptr) &&
gtTreeHasSideEffects(arg.GetEarlyNode(), flags, ignoreCctors))
{
return true;
}

if ((arg.GetLateNode() != nullptr) && gtTreeHasSideEffects(arg.GetLateNode(), flags, ignoreCctors))
{
return true;
}
}

// Otherwise:
return false;
}

// Otherwise the GT_CALL is considered to have side-effects.
return true;
return call->HasSideEffects(this, ignoreExceptions, ignoreCctors);
}
}

Expand Down Expand Up @@ -16892,19 +16868,30 @@ bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_S

if (sideEffectFlags == GTF_CALL)
{
if (tree->OperGet() == GT_CALL)
if (tree->IsHelperCall())
{
// Generally all trees that contain GT_CALL nodes are considered to have side-effects.
//
if (tree->AsCall()->IsHelperCall())
// However, for some pure helper calls we lie about this.
if (gtNodeHasSideEffects(tree, flags, ignoreCctors))
{
// If this node is a helper call we may not care about the side-effects.
// Note that gtNodeHasSideEffects checks the side effects of the helper itself
// as well as the side effects of its arguments.
return gtNodeHasSideEffects(tree, flags, ignoreCctors);
return true;
}

// The GTF_CALL may be contributed by an operand, so check for
// that.
bool hasCallInOperand = false;
tree->VisitOperands([=, &hasCallInOperand](GenTree* op) {
if (gtTreeHasSideEffects(op, GTF_CALL, ignoreCctors))
{
hasCallInOperand = true;
return GenTree::VisitResult::Abort;
}
return GenTree::VisitResult::Continue;
});

return hasCallInOperand;
}
else if (tree->OperGet() == GT_INTRINSIC)
else if (tree->OperIs(GT_INTRINSIC))
{
if (gtNodeHasSideEffects(tree, flags, ignoreCctors))
{
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1666,13 +1666,16 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call)
unsigned tmpVarNum = comp->lvaGrabTemp(true DEBUGARG("argument with side effect"));
GenTree* store = comp->gtNewTempStore(tmpVarNum, use.GetNode());

INDEBUG(store->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

if (setupArg == nullptr)
{
setupArg = store;
}
else
{
setupArg = comp->gtNewOperNode(GT_COMMA, TYP_VOID, setupArg, store);
INDEBUG(setupArg->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
}

use.SetNode(comp->gtNewLclvNode(tmpVarNum, genActualType(use.GetNode())));
Expand All @@ -1688,6 +1691,8 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call)

setupArg = comp->gtNewTempStore(tmpVarNum, argx);

INDEBUG(setupArg->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

LclVarDsc* varDsc = comp->lvaGetDesc(tmpVarNum);
var_types lclVarType = genActualType(argx->gtType);
var_types scalarType = TYP_UNKNOWN;
Expand Down
Loading