Structs: Do not mark "Hidden buffer pointer" as address-exposed #64130
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailsnull
|
kunalspathak
changed the title
kunalspathak
force-pushed
the
hidden_buffer
branch
from
1053d52
to
50f27ba
Compare
|
@dotnet/jit-contrib @SingleAccretion curious to get your thoughts here. @kunalspathak you may need to write up an overview of what you're up to. |
|
/azp run superpmi-replay |
|
No pipelines are associated with this pull request. |
|
CoreClr Pri0 test failures are related to #63325 |
|
/azp list |
|
/azp run runtime-coreclr jitstress |
|
/azp run runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr jitstress |
|
No pipelines are associated with this pull request. |
|
/azp run runtime-coreclr jitstress |
1 similar comment
|
/azp run runtime-coreclr jitstress |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 similar comments
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kunalspathak
force-pushed
the
hidden_buffer
branch
from
b992ec6
to
9cb8989
Compare
| GenTree* GetLclRetBufArgNode() const | ||
| { | ||
| if (gtRetBufArg == nullptr) | ||
| { |
There was a problem hiding this comment.
Should we assert on both sides of this? (that the flag and the class member agree)
| { | |
| { | |
| assert(!HasRetBufArg()); |
There was a problem hiding this comment.
Turns out that when we get RetBuf as argument in a register (as I understand), and if that RetBuf is further used in a call in current method, that argument is propagated as-is without having to put it on current frame:
[000000] S-C-G------- * CALL void hackishModuleName.hackishMethodName
[000001] ------------ arg0 \--* LCL_VAR byref V01 RetBuf
In such cases, we are not passing the address and hence we don't perform the optimization of this PR. For such scenarios, gtRetBufArg == nullptr while HasRetBufArg() is true. We could extract the first argument and then validate for such cases if its varDsc->lvIsRegArg == true, but not sure if it is worth it.
There was a problem hiding this comment.
Do you have a simple example where this happens?
There was a problem hiding this comment.
In the current design gtRetBufArg is only set if the buffer's address is local.
(Simple case where it is not: *globalPtr = CallThatReturnsStruct();).
This is mainly for throughput reasons -- we'd like to be able to early out of GetLclRetBufArgNode.
There was a problem hiding this comment.
|
asmdiffs summary after all PR review updates:
|
|
Improvements in dotnet/perf-autofiling-issues#4224 |
…et#64130) * Preliminary changes * Add the missing push of ssadef * Fix an issue for call tree cloning * formatting * minor review comments incorporation * Make GT_CALL as full-def * Merge RenameCall into RenameDef * Fix DefinesLocalAddr and lateuse arg * Minor comments * fix the recently added code to account that GT_CALL can also have defintion: * clone the return buffer arg * Another round of review comments * Handle return buffer calls in optIsVarAssgCB * Update locals defined by calls in NodeInfo * Fix a case where arg can be putarg * Restructure the cases in GetLclRetBufArgNode() * move ivaMaskCall outside the condition * Add back to call for DoNotEnregister which was missed for Release builds * Retrieve the appropriate node in case of copyReload * Added an assert * remove assert and add comment
ghost
locked as resolved and limited conversation to collaborators
When a method returns a struct, the caller often passes the location of its stack frame as one of the argument, where callee can store the struct it would return. This argument is known as "hidden buffer" argument. The callee would store the struct it should return into this hidden buffer location. When the callee returns, the caller copies the struct value from the hidden buffer into another stack frame location which subsequently is used to access the struct. Currently, we mark such argument as "address-exposed", which prevent us from doing further optimizations on the hidden buffer. However, there is no IL / user code that would modify or pollute the hidden buffer and is strictly used by .NET ABI. Hence, it is safe to assume that the address of the hidden buffer is not exposed even if it is passed to the callee. This opens up opportunities of optimizing the hidden buffer location and eliminating the extra copy that the caller has to perform after the callee returns.
Here is an example taken from #12219
@benaadams
Here is the summary of no. of methods improved/regressed in all SPMI collections.
I studied some regressions and they are one of the two categories:
Changes:
LocalAddressVisitor, before we mark anything with address-exposed, we would check if it is "hidden buffer" and if yes, setlvHiddenBufferStructArg = true. Since this is a unique node that is defined (written by the callee) and used (passed as argument by the caller), it is marked as partial definition usinggtFlags |= (GTF_VAR_DEF | GTF_VAR_USEASG).LclSsaVarDscis written considering thatGT_ASGcan be the only node that introduces a definition. However, with this PR, we will also introduce a definition forGT_CALLnodes. AddedRenameCall()method that handles call nodes and assigns SSA to the hidden buffer argument.COMPlus_JitHiddenBufferForStructthat is enabled by default but can be turned off for easy debugging.References: