Remove stores as operands of calls in LIR (#68460)

These are part of operands in HIR simply for ordering purposes, which is
no longer necessary in LIR. This means that every argument operand in
LIR now has exactly one node, which between rationalization and lowering
is always a value.

We still have cases of operands that are non-values, in particular the
operand of GT_JTRUE nodes and also GT_PUTARG_STK nodes after lowering.
However, at least for calls the LIR invariant is now simpler: before
lowering all argument operands are values, and after all argument
operands are GT_PUTARG_* nodes.

Not having stores as operands will also allow us to get rid of
GTF_LATE_ARG in a follow-up change.

Author: jakobbotsch

src/coreclr/jit/gentree.cpp

@@ -17867,7 +17867,7 @@ bool Compiler::gtIsStaticGCBaseHelperCall(GenTree* tree)
 
 //------------------------------------------------------------------------
 // gtCallGetDefinedRetBufLclAddr:
-//   Get the tree corresponding to the address of the retbuf taht this call defines.
+//   Get the tree corresponding to the address of the retbuf that this call defines.
 //
 // Parameters:
 //   call - The call node

src/coreclr/jit/lir.cpp

@@ -1557,10 +1557,8 @@ bool LIR::Range::CheckLIR(Compiler* compiler, bool checkUnusedValues) const
                 // Stack arguments do not produce a value, but they are considered children of the call.
                 // It may be useful to remove these from being call operands, but that may also impact
                 // other code that relies on being able to reach all the operands from a call node.
-                // The GT_NOP case is because sometimes we eliminate stack argument stores as dead, but
-                // instead of removing them we replace with a NOP.
                 // The argument of a JTRUE doesn't produce a value (just sets a flag).
-                assert(((node->OperGet() == GT_CALL) && (def->OperIsStore() || def->OperIs(GT_PUTARG_STK, GT_NOP))) ||
+                assert(((node->OperGet() == GT_CALL) && def->OperIs(GT_PUTARG_STK)) ||
                        ((node->OperGet() == GT_JTRUE) && (def->TypeGet() == TYP_VOID) &&
                         ((def->gtFlags & GTF_SET_FLAGS) != 0)));
                 continue;

src/coreclr/jit/liveness.cpp

@@ -2205,35 +2205,17 @@ bool Compiler::fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange)
 }
 
 //---------------------------------------------------------------------
-// fgRemoveDeadStoreSimple - remove a dead store
+// fgRemoveDeadStoreLIR - remove a dead store from LIR
 //
-//   pTree          - GenTree** to local, including store-form local or local addr (post-rationalize)
-//   varDsc         - var that is being stored to
-//   life           - current live tracked vars (maintained as we walk backwards)
-//   doAgain        - out parameter, true if we should restart the statement
-//   pStmtInfoDirty - should defer the cost computation to the point after the reverse walk is completed?
+//   store          - A store tree
+//   block          - Block that the store is part of
 //
 void Compiler::fgRemoveDeadStoreLIR(GenTree* store, BasicBlock* block)
 {
     LIR::Range& blockRange = LIR::AsRange(block);
 
-    // If the store is marked as a late argument, it is referenced by a call.
-    // Instead of removing it, bash it to a NOP.
-    if ((store->gtFlags & GTF_LATE_ARG) != 0)
-    {
-        JITDUMP("node is a late arg; replacing with NOP\n");
-        store->gtBashToNOP();
-
-        // NOTE: this is a bit of a hack. We need to keep these nodes around as they are
-        // referenced by the call, but they're considered side-effect-free non-value-producing
-        // nodes, so they will be removed if we don't do this.
-        store->gtFlags |= GTF_ORDER_SIDEEFF;
-    }
-    else
-    {
-        blockRange.Remove(store);
-    }
-
+    assert((store->gtFlags & GTF_LATE_ARG) == 0);
+    blockRange.Remove(store);
     assert(!opts.MinOpts());
     fgStmtRemoved = true;
 }

src/coreclr/jit/lower.cpp

@@ -1343,20 +1343,7 @@ void Lowering::LowerArg(GenTreeCall* call, CallArg* callArg, bool late)
 
     JITDUMP("lowering arg : ");
     DISPNODE(arg);
-
-    // No assignments should remain by Lowering.
-    assert(!arg->OperIs(GT_ASG));
-    assert(!arg->OperIsPutArgStk());
-
-    // Assignments/stores at this level are not really placing an argument.
-    // They are setting up temporary locals that will later be placed into
-    // outgoing regs or stack.
-    // Note that atomic ops may be stores and still produce a value.
-    if (!arg->IsValue())
-    {
-        assert((arg->OperIsStore() && !arg->IsValue()) || arg->IsNothingNode() || arg->OperIsCopyBlkOp());
-        return;
-    }
+    assert(arg->IsValue());
 
     var_types type = arg->TypeGet();
 
@@ -6425,7 +6412,7 @@ void Lowering::CheckCallArg(GenTree* arg)
 {
     if (!arg->IsValue() && !arg->OperIsPutArgStk())
     {
-        assert(arg->OperIsStore() || arg->IsNothingNode() || arg->OperIsCopyBlkOp());
+        assert(arg->OperIsStore() || arg->OperIsCopyBlkOp());
         return;
     }
 

src/coreclr/jit/rationalize.cpp

@@ -615,6 +615,28 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge
             RewriteIndir(use);
             break;
 
+        case GT_CALL:
+            // In linear order we no longer need to retain the stores in early
+            // args as these have now been sequenced.
+            for (CallArg& arg : node->AsCall()->gtArgs.EarlyArgs())
+            {
+                if (!arg.GetEarlyNode()->IsValue())
+                {
+                    // GTF_LATE_ARG is no longer meaningful.
+                    arg.GetEarlyNode()->gtFlags &= ~GTF_LATE_ARG;
+                    arg.SetEarlyNode(nullptr);
+                }
+            }
+
+#ifdef DEBUG
+            // The above means that all argument nodes are now true arguments.
+            for (CallArg& arg : node->AsCall()->gtArgs.Args())
+            {
+                assert((arg.GetEarlyNode() == nullptr) != (arg.GetLateNode() == nullptr));
+            }
+#endif
+            break;
+
         case GT_NOP:
             // fgMorph sometimes inserts NOP nodes between defs and uses
             // supposedly 'to prevent constant folding'. In this case, remove the
@@ -637,8 +659,8 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge
             if ((sideEffects & GTF_ALL_EFFECT) == 0)
             {
                 // The LHS has no side effects. Remove it.
-                // None of the transforms performed herein violate tree order, so isClosed
-                // should always be true.
+                // All transformations on pure trees keep their operands in LIR
+                // and should not violate tree order.
                 assert(isClosed);
 
                 BlockRange().Delete(comp, m_block, std::move(lhsRange));
@@ -666,8 +688,8 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge
 
                 if ((sideEffects & GTF_ALL_EFFECT) == 0)
                 {
-                    // None of the transforms performed herein violate tree order, so isClosed
-                    // should always be true.
+                    // All transformations on pure trees keep their operands in
+                    // LIR and should not violate tree order.
                     assert(isClosed);
 
                     BlockRange().Delete(comp, m_block, std::move(rhsRange));