From 15e96faf4558b017ea8df1dc28d9b2169f0badc0 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 12 Aug 2024 11:37:54 +0200 Subject: [PATCH] JIT: Fix `gtNodeHasSideEffects` checking call arguments (#106185) `gtNodeHasSideEffects` is meant to check if a node has side effects when you exclude its children. However, it was checking arguments of calls which is more conservative than expected. The actual reason we were doing that seems to be `gtTreeHasSideEffects` that sometimes wants to ignore `GTF_CALL` on pure helper calls. It was relying on this check in `gtNodeHasSideEffect`; instead move it to `gtTreeHasSideEffects` where it belongs. This is an alternative fix for #106129; there we leave a `COMMA(CORINFO_HELP_RUNTIMELOOKUP_METHOD, ...)` around because extracting side effects from op1 does not end up getting rid of the call. Fix #106129 --- src/coreclr/jit/assertionprop.cpp | 8 +---- src/coreclr/jit/gentree.cpp | 53 ++++++++++++------------------- src/coreclr/jit/morph.cpp | 5 +++ 3 files changed, 26 insertions(+), 40 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index db1ea44806787..3d61a698a08d5 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -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 diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index a7dda630e475d..cb4b1b261ac0a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -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); } } @@ -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)) { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index f773f500697db..d037282e447ab 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1666,6 +1666,8 @@ 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; @@ -1673,6 +1675,7 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call) 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()))); @@ -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;