Remove stores as operands of calls in LIR #68460
Conversation
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.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThese are part of operands in HIR simply for ordering purposes, which is We still have cases of operands that are non-values, in particular the Not having stores as operands will also allow us to get rid of cc @dotnet/jit-contrib
|
| // | ||
| 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) |
There was a problem hiding this comment.
From a cursory glance this use of GTF_LATE_ARG is (was) the only blocker of its removal.
| arg.GetEarlyNode()->gtFlags &= ~GTF_LATE_ARG; | ||
| arg.SetEarlyNode(nullptr); |
There was a problem hiding this comment.
GTF_CALL_M_RETBUFFARG_LCLOPT has a dependency on being able to reach the setup node from the call.
Currently it will work ok because liveness will count the LCL_VAR|FLD_ADDR as the definition point, but that itself is only fine because GTF_CALL_M_RETBUFFARG_LCLOPT is tied to lvIsTemp.
Would be curious to know your thinking on this; my idea was to forbid forcing the buffer arguments to temps.
There was a problem hiding this comment.
I'm surprised it would ever be evaluated to a temp since it should be invariant. But yes, we should avoid creating a temp for it.
There was a problem hiding this comment.
I'm surprised it would ever be evaluated to a temp since it should be invariant
Agree, it is mostly a "model" concern, without GTF_CALL_M_RETBUFFARG_LCLOPT forcing things to temps is always legal, with it (+ this change), it isn't.
There was a problem hiding this comment.
I opened #68469 to stop it looking into the stores and to assert the contract more heavily.
|
|
|
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
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.
cc @dotnet/jit-contrib