From f6d8738aaf7a85d0635d143fd4a89469dc57319e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 24 Apr 2022 13:29:06 +0200 Subject: [PATCH 1/3] Remove stores as operands of calls in LIR 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. --- src/coreclr/jit/lir.cpp | 2 +- src/coreclr/jit/liveness.cpp | 28 +++++----------------------- src/coreclr/jit/lower.cpp | 15 +-------------- src/coreclr/jit/rationalize.cpp | 30 ++++++++++++++++++++++++++---- 4 files changed, 33 insertions(+), 42 deletions(-) diff --git a/src/coreclr/jit/lir.cpp b/src/coreclr/jit/lir.cpp index 212861a1e5861..fbe385122a4ab 100644 --- a/src/coreclr/jit/lir.cpp +++ b/src/coreclr/jit/lir.cpp @@ -1560,7 +1560,7 @@ bool LIR::Range::CheckLIR(Compiler* compiler, bool checkUnusedValues) const // 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; diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index 51996354297a1..517d14fa1bf9f 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/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; } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 87ff5a1d2aa6b..7108c284b6b65 100644 --- a/src/coreclr/jit/lower.cpp +++ b/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(); diff --git a/src/coreclr/jit/rationalize.cpp b/src/coreclr/jit/rationalize.cpp index bc0032e19b696..04517a5993582 100644 --- a/src/coreclr/jit/rationalize.cpp +++ b/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)); From a046b08661dfb7b480a029e57171f18be3412c4e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 24 Apr 2022 13:47:07 +0200 Subject: [PATCH 2/3] Fix stale comment --- src/coreclr/jit/lir.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/lir.cpp b/src/coreclr/jit/lir.cpp index fbe385122a4ab..6fec33642386c 100644 --- a/src/coreclr/jit/lir.cpp +++ b/src/coreclr/jit/lir.cpp @@ -1557,8 +1557,6 @@ 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->OperIs(GT_PUTARG_STK)) || ((node->OperGet() == GT_JTRUE) && (def->TypeGet() == TYP_VOID) && From 7d6e55aff9e153cfb2ab8e9ad68a53ccd4d4e4c5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 26 Apr 2022 19:29:15 +0200 Subject: [PATCH 3/3] Fix a comment and assert --- src/coreclr/jit/gentree.cpp | 2 +- src/coreclr/jit/lower.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 4a798a74cd9d9..1a5d3a1b716df 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17866,7 +17866,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 diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index dcebbdcb06bfa..7108c284b6b65 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -6412,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; }