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

Author: jakobbotsch

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

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))
             {

src/coreclr/jit/morph.cpp

@@ -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())));
@@ -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;