Decouple call store operands from local ret buf optimization #68469

jakobbotsch · 2022-04-24T17:59:55Z

Move GenTreeCall::GetLclRetBufArgNode ->
Compiler::gtCallGetDefinedRetBufLclAddr and change it to not try to
look into stores
Assert that the node we are returning actually defines a local that
has lvHiddenBufferStructArg set
Assert that call morphing does not break our recognition of the defined
local when optimizing
Update Compiler::DefinesLocalAddr to check for GT_LCL_FLD_ADDR

* Move GenTreeCall::GetLclRetBufArgNode -> Compiler::gtCallGetDefinedRetBufLclAddr and change it to not try to look into stores * Assert that the node we are returning actually defines a local that has lvHiddenBufferStructArg set * Assert that call morphing does break our recognition of the defined local when optimizing

ghost · 2022-04-24T18:00:02Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Move GenTreeCall::GetLclRetBufArgNode ->
Compiler::gtCallGetDefinedRetBufLclAddr and change it to not try to
look into stores
Assert that the node we are returning actually defines a local that
has lvHiddenBufferStructArg set
Assert that call morphing does break our recognition of the defined
local when optimizing
Update Compiler::DefinesLocalAddr to check for GT_LCL_FLD_ADDR

Author:	jakobbotsch
Assignees:	jakobbotsch
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion

Thank you for addressing this!

I would say it would probably be prudent to run some stress over this to avoid surprises with struct field addresses being GLOB_REF-ed.

SingleAccretion · 2022-04-24T18:54:47Z

src/coreclr/jit/gentree.cpp

+    // This may be called very late to check validity of LIR.
+    if (node->IsCopyOrReload())
+    {
+        node = node->AsCopyOrReload()->gtGetOp1();
+    }


Nit: this is gtSkipReloadOrCopy.

SingleAccretion · 2022-04-24T19:01:56Z

src/coreclr/jit/morph.cpp

@@ -7453,7 +7457,7 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL
        JITDUMP("Removing retbuf");

        call->gtArgs.Remove(call->gtArgs.GetRetBufferArg());
-        call->gtCallMoreFlags &= ~GTF_CALL_M_RETBUFFARG;
+        call->gtCallMoreFlags &= ~(GTF_CALL_M_RETBUFFARG | GTF_CALL_M_RETBUFFARG_LCLOPT);


In general the rule is that if you remove GTF_CALL_M_RETBUFFARG_LCLOPT, you need to address-expose the underlying local (since it makes it invisible to liveness), I suppose here we can elide that because the logical definition occurs outside the method (since it is a tailcall)?

Perhaps worth a comment (or not - maybe it is self-evident).

Right, things have already gone very wrong if we think we have a tailcall with a retbuf that can be optimized with this optimization.
I will just revert this change, we will hit the assert inside of gtCallGetDefinedRetBufLclAddr if the flag is set and we don't have a retbuf.

SingleAccretion · 2022-04-24T19:17:00Z

src/coreclr/jit/gentree.cpp

@@ -16180,7 +16180,7 @@ bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bo
 // Returns true if this GenTree defines a result which is based on the address of a local.
 bool GenTree::DefinesLocalAddr(Compiler* comp, unsigned width, GenTreeLclVarCommon** pLclVarTree, bool* pIsEntire)
 {
-    if (OperGet() == GT_ADDR || OperGet() == GT_LCL_VAR_ADDR)
+    if (OperIs(GT_ADDR, GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR))


Nit: this local morph comment is now outdated.

Do you know why DefinesLocalAddr was not recognizing the pattern in the first place? Since it supports communicating "entirety" it seems pretty uncontroversial.

Do you know why DefinesLocalAddr was not recognizing the pattern in the first place?

Not really I suppose. If I were to guess, it is the result of LCL_FLD being a somewhat late addition, and the original version of the function only recognizing ADDR(LCL_VAR).

I do believe there are no correctness dependencies on it not recognizing LCL_FLD_ADDR.

(Side note: the contract of DefinesLocalAddr is that it must recognize a superset of addresses LocalAddressVisitor recognizes, absence of FLD_ADDR seemingly violates this, but not actually, because it does recognize ADDR(LCL_FLD), and in LIR we rely on use-def flags set by the front-end)

jakobbotsch · 2022-04-25T12:18:58Z

~~This is currently waiting for #68484.~~

jakobbotsch · 2022-04-25T20:38:35Z

/azp run runtime-coreclr jitstress, runtime-coreclr outerloop

azure-pipelines · 2022-04-25T20:38:54Z

Azure Pipelines successfully started running 2 pipeline(s).

kunalspathak

Looks good, just a question in sideeffects.cpp.

kunalspathak · 2022-04-26T07:04:50Z

src/coreclr/jit/gentree.cpp

@@ -17864,6 +17864,57 @@ bool Compiler::gtIsStaticGCBaseHelperCall(GenTree* tree)
    return false;
 }

+//------------------------------------------------------------------------
+// gtCallGetDefinedRetBufLclAddr:
+//   Get the tree corresponding to the address of the retbuf taht this call defines.


Suggested change

// 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.

Let me include it as part of #68460 unless there are other changes.

kunalspathak · 2022-04-26T07:11:33Z

src/coreclr/jit/sideeffects.cpp

        if (retBufArgNode != nullptr)
        {
-            // If a copy/reload is inserted by LSRA, retrieve the returnBuffer


Why is this no longer needed? I don't recall which test, but I added it much later when trying to debug a test failure.

I have moved it into gtCallGetDefinedRetBufLclAddr instead, since it also needs to skip it for the assertion there.

Ah, didn't notice that. Thanks!

jakobbotsch · 2022-04-26T14:03:50Z

Some of the helix jobs on outerloop/jitstress windows-arm64 machines timed out. Not sure why as the job lists all seem to have "State": "Finished", but in previous runs they sometimes take a very long time too and there are some timeouts previously too, so will assume it's unrelated to this change.

OSX x64 failure is #67816.

kunalspathak

LGTM

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 24, 2022

ghost assigned jakobbotsch Apr 24, 2022

jakobbotsch mentioned this pull request Apr 24, 2022

Remove stores as operands of calls in LIR #68460

Merged

SingleAccretion reviewed Apr 24, 2022

View reviewed changes

SingleAccretion mentioned this pull request Apr 25, 2022

Recognize more "local addr" trees as invariant #68484

Merged

runfoapp bot mentioned this pull request Apr 25, 2022

Test failure tracing/eventpipe/providervalidation/providervalidation/providervalidation.sh #59296

Closed

jakobbotsch requested a review from kunalspathak April 25, 2022 19:25

jakobbotsch added 2 commits April 25, 2022 21:31

Address feedback

4338d32

Merge branch 'main' of github.com:dotnet/runtime into retbuf-def-asserts

fabebd4

kunalspathak requested changes Apr 26, 2022

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Apr 26, 2022

kunalspathak approved these changes Apr 26, 2022

View reviewed changes

jakobbotsch merged commit ae9f1ca into dotnet:main Apr 26, 2022

jakobbotsch deleted the retbuf-def-asserts branch April 26, 2022 16:04

ghost locked as resolved and limited conversation to collaborators May 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple call store operands from local ret buf optimization #68469

Decouple call store operands from local ret buf optimization #68469

jakobbotsch commented Apr 24, 2022 •

edited

Loading

ghost commented Apr 24, 2022

SingleAccretion left a comment

SingleAccretion Apr 24, 2022

SingleAccretion Apr 24, 2022

jakobbotsch Apr 24, 2022 •

edited

Loading

SingleAccretion Apr 24, 2022

jakobbotsch Apr 24, 2022

SingleAccretion Apr 24, 2022 •

edited

Loading

jakobbotsch commented Apr 25, 2022 •

edited

Loading

jakobbotsch commented Apr 25, 2022

azure-pipelines bot commented Apr 25, 2022

kunalspathak left a comment

kunalspathak Apr 26, 2022

jakobbotsch Apr 26, 2022

kunalspathak Apr 26, 2022

jakobbotsch Apr 26, 2022

kunalspathak Apr 26, 2022

jakobbotsch commented Apr 26, 2022

kunalspathak left a comment

	// 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.

Decouple call store operands from local ret buf optimization #68469

Decouple call store operands from local ret buf optimization #68469

Conversation

jakobbotsch commented Apr 24, 2022 • edited Loading

ghost commented Apr 24, 2022

SingleAccretion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SingleAccretion Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

jakobbotsch commented Apr 25, 2022 • edited Loading

jakobbotsch commented Apr 25, 2022

azure-pipelines bot commented Apr 25, 2022

kunalspathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Apr 26, 2022

kunalspathak left a comment

Choose a reason for hiding this comment

jakobbotsch commented Apr 24, 2022 •

edited

Loading

jakobbotsch Apr 24, 2022 •

edited

Loading

SingleAccretion Apr 24, 2022 •

edited

Loading

jakobbotsch commented Apr 25, 2022 •

edited

Loading