-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
} | ||||||
|
@@ -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; | ||||||
|
@@ -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)) | ||||||
{ | ||||||
GenTree* addrArg = this; | ||||||
if (OperGet() == GT_ADDR) | ||||||
|
@@ -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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
node = node->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. | ||||||
// | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have moved it into There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 recognizingADDR(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 addressesLocalAddressVisitor
recognizes, absence ofFLD_ADDR
seemingly violates this, but not actually, because it does recognizeADDR(LCL_FLD)
, and in LIR we rely on use-def flags set by the front-end)