Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple call store operands from local ret buf optimization #68469

Merged
merged 3 commits into from
Apr 26, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -2790,6 +2790,8 @@ class Compiler
// Check if this tree is a gc static base helper call
bool gtIsStaticGCBaseHelperCall(GenTree* tree);

GenTree* gtCallGetDefinedRetBufLclAddr(GenTreeCall* call);

//-------------------------------------------------------------------------
// Functions to display the trees

Expand Down
60 changes: 57 additions & 3 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6273,7 +6273,7 @@ bool GenTree::OperRequiresAsgFlag()
{
// If the call has return buffer argument, it produced a definition and hence
// should be marked with assignment.
return AsCall()->GetLclRetBufArgNode() != nullptr;
return AsCall()->IsOptimizingRetBufAsLocal();
}
return false;
}
Expand Down Expand Up @@ -16138,7 +16138,7 @@ bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bo
}
else if (OperIs(GT_CALL))
{
GenTree* retBufArg = AsCall()->GetLclRetBufArgNode();
GenTree* retBufArg = comp->gtCallGetDefinedRetBufLclAddr(AsCall());
if (retBufArg == nullptr)
{
return false;
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this local morph comment is now outdated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@SingleAccretion SingleAccretion Apr 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
GenTree* addrArg = this;
if (OperGet() == GT_ADDR)
Expand Down Expand Up @@ -17864,6 +17864,60 @@ bool Compiler::gtIsStaticGCBaseHelperCall(GenTree* tree)
return false;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//
// Parameters:
// call - The call node
//
// Returns:
// A tree representing the address of a local.
//
// Remarks:
// This function should not be used until after morph when local address
// nodes have been normalized. However, before that IsOptimizingRetBufAsLocal
// can be used to at least check if the call has a retbuf that we are
// optimizing.
//
GenTree* Compiler::gtCallGetDefinedRetBufLclAddr(GenTreeCall* call)
{
if (!call->IsOptimizingRetBufAsLocal())
{
return nullptr;
}

CallArg* retBufArg = call->gtArgs.GetRetBufferArg();
assert(retBufArg != nullptr);

GenTree* node = retBufArg->GetNode();
switch (node->OperGet())
{
// Get the value from putarg wrapper nodes
case GT_PUTARG_REG:
case GT_PUTARG_STK:
node = node->AsOp()->gtGetOp1();
break;

default:
break;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is gtSkipReloadOrCopy.


#ifdef DEBUG
unsigned size = typGetObjLayout(call->gtRetClsHnd)->GetSize();
GenTreeLclVarCommon* lcl;
assert(node->DefinesLocalAddr(this, size, &lcl, nullptr) && lvaGetDesc(lcl)->lvHiddenBufferStructArg);
#endif

return node;
}

//------------------------------------------------------------------------
// ParseArrayAddress: Rehydrate the array and index expression from ARR_ADDR.
//
Expand Down
38 changes: 5 additions & 33 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -5031,6 +5031,11 @@ struct GenTreeCall final : public GenTree
return (gtCallMoreFlags & GTF_CALL_M_EXPANDED_EARLY) != 0;
}

bool IsOptimizingRetBufAsLocal()
{
return (gtCallMoreFlags & GTF_CALL_M_RETBUFFARG_LCLOPT) != 0;
}

//-----------------------------------------------------------------------------------------
// GetIndirectionCellArgKind: Get the kind of indirection cell used by this call.
//
Expand Down Expand Up @@ -5177,39 +5182,6 @@ struct GenTreeCall final : public GenTree
{
}
#endif

GenTree* GetLclRetBufArgNode()
{
if (!gtArgs.HasRetBuffer() || ((gtCallMoreFlags & GTF_CALL_M_RETBUFFARG_LCLOPT) == 0))
{
return nullptr;
}

CallArg* retBufArg = gtArgs.GetRetBufferArg();
GenTree* lclRetBufArgNode = retBufArg->GetEarlyNode();
if (lclRetBufArgNode == nullptr)
{
lclRetBufArgNode = retBufArg->GetLateNode();
}

switch (lclRetBufArgNode->OperGet())
{
// Get the true value from setup args
case GT_ASG:
return lclRetBufArgNode->AsOp()->gtGetOp2();
case GT_STORE_LCL_VAR:
return lclRetBufArgNode->AsUnOp()->gtGetOp1();

// Get the value from putarg wrapper nodes
case GT_PUTARG_REG:
case GT_PUTARG_STK:
return lclRetBufArgNode->AsOp()->gtGetOp1();

// Otherwise the node should be in the Use*
default:
return lclRetBufArgNode;
}
}
};

struct GenTreeCmpXchg : public GenTree
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1782,6 +1782,10 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call)
lateTail = &arg.LateNextRef();
}

// Make sure we did not do anything here that stops us from being able to
// find the local ret buf if we are optimizing it.
noway_assert(!call->IsOptimizingRetBufAsLocal() || (comp->gtCallGetDefinedRetBufLclAddr(call) != nullptr));

#ifdef DEBUG
if (comp->verbose)
{
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jakobbotsch jakobbotsch Apr 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

const bool stubNeedsTargetFnPtr = (help.flags & CORINFO_TAILCALL_STORE_TARGET) != 0;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5533,7 +5533,7 @@ Compiler::fgWalkResult Compiler::optIsVarAssgCB(GenTree** pTree, fgWalkData* dat
{
desc->ivaMaskCall = optCallInterf(tree->AsCall());

dest = tree->AsCall()->GetLclRetBufArgNode();
dest = data->compiler->gtCallGetDefinedRetBufLclAddr(tree->AsCall());
if (dest == nullptr)
{
return WALK_CONTINUE;
Expand Down
8 changes: 1 addition & 7 deletions src/coreclr/jit/sideeffects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,15 +141,9 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node)
if (node->IsCall())
{
// For calls having return buffer, update the local number that is written after this call.
GenTree* retBufArgNode = node->AsCall()->GetLclRetBufArgNode();
GenTree* retBufArgNode = compiler->gtCallGetDefinedRetBufLclAddr(node->AsCall());
if (retBufArgNode != nullptr)
{
// If a copy/reload is inserted by LSRA, retrieve the returnBuffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't notice that. Thanks!

if (retBufArgNode->IsCopyOrReload())
{
retBufArgNode = retBufArgNode->AsCopyOrReload()->gtGetOp1();
}

m_flags |= ALIAS_WRITES_LCL_VAR;
m_lclNum = retBufArgNode->AsLclVarCommon()->GetLclNum();
m_lclOffs = retBufArgNode->AsLclVarCommon()->GetLclOffs();
Expand Down