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.

Sign up for GitHub

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

Jump to bottom

Adjust impNormStructVal to not wrap in GT_OBJ #81636

Merged
merged 3 commits into from
Feb 6, 2023
Merged

Adjust impNormStructVal to not wrap in GT_OBJ #81636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d55a004
Adjust impNormStructVal to not wrap in GT_OBJ
tannergooding Feb 4, 2023
cd579dc
Ensure UpdateEarlyRefCount handles CALL(LCL_VAR)
tannergooding Feb 5, 2023
bc306b1
Merge remote-tracking branch 'dotnet/main' into impNormStructVal
tannergooding Feb 5, 2023
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
11 changes: 0 additions & 11 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,11 +1332,6 @@ GenTree* Compiler::impNormStructVal(GenTree* structVal, CORINFO_CLASS_HANDLE str

case GT_LCL_VAR:
case GT_LCL_FLD:
structLcl = structVal->AsLclVarCommon();
// Wrap it in a GT_OBJ.
structVal = gtNewObjNode(structHnd, gtNewOperNode(GT_ADDR, TYP_BYREF, structVal));
FALLTHROUGH;

case GT_IND:
case GT_OBJ:
case GT_BLK:
Expand Down Expand Up @@ -1414,12 +1409,6 @@ GenTree* Compiler::impNormStructVal(GenTree* structVal, CORINFO_CLASS_HANDLE str

structLcl = gtNewLclvNode(tmpNum, structType)->AsLclVarCommon();
structVal = structLcl;

if (structType == TYP_STRUCT)
{
// Wrap it in a GT_OBJ
structVal = gtNewObjNode(structHnd, gtNewOperNode(GT_ADDR, TYP_BYREF, structVal));
}
}

if (structLcl != nullptr)
Expand Down
56 changes: 29 additions & 27 deletions src/coreclr/jit/lclmorph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1565,40 +1565,42 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
// handled in fgCanFastTailCall and fgMakeOutgoingStructArgCopy.
//
// CALL(OBJ(LCL_VAR_ADDR...))
bool isArgToCall = false;
bool keepSearching = true;
for (int i = 0; i < m_ancestors.Height() && keepSearching; i++)
// -or-
// CALL(LCL_VAR)

// TODO-1stClassStructs: We've removed most, but not all, cases where OBJ(LCL_VAR_ADDR)
// is introduced (it was primarily from impNormStructVal). But until all cases are gone
// we still want to handle it as well.

if (m_ancestors.Height() < 2)
Copy link
Member Author

@tannergooding tannergooding Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

UpdateEarlyRefCount needs to recognize LCL_VARs.

Updated that here. I left in the OBJ(LCL_VAR_ADDR) handling since we still have that in some cases (even if the majority of OBJ wrapping is gone now that it's out of impNormStrutVal.

Also changed it from a for loop to to some explicit linear checks instead, which simplifies things and should improve the method TP.

Copy link
Member Author

Choose a reason for hiding this comment

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

-- Not the most familiar with LclMorph, so I went with the assumption that the early exit for !m_compiler->lvaIsImplicitByRefLocal(lclNum) is enough to ensure LCL_VAR is something we want to handle and that we don't also need to explicitly check that LCL_VAR is a struct or a struct that would actually be passed by ref (e.g. struct S { int x; } typically gets passed in register instead).

Let me know if this assumption isn't correct and if more checks are needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also did not handle LCL_FLD since this is "pre-order" and I believe that means we should only see GT_FIELD instead at this point in time.

{
return;
}

GenTree* node = m_ancestors.Top(0);

if (node->OperIs(GT_LCL_VAR))
{
GenTree* node = m_ancestors.Top(i);
switch (i)
node = m_ancestors.Top(1);
}
else if (node->OperIs(GT_LCL_VAR_ADDR))
{
if (m_ancestors.Height() < 3)
{
case 0:
{
keepSearching = node->OperIs(GT_LCL_VAR_ADDR);
}
break;
return;
}

case 1:
{
keepSearching = node->OperIs(GT_OBJ);
}
break;
node = m_ancestors.Top(1);

case 2:
{
keepSearching = false;
isArgToCall = node->IsCall();
}
break;
default:
{
keepSearching = false;
}
break;
if (!node->OperIs(GT_OBJ))
{
return;
}

node = m_ancestors.Top(2);
}

if (isArgToCall)
if (node->IsCall())
{
JITDUMP("LocalAddressVisitor incrementing weighted ref count from " FMT_WT " to " FMT_WT
" for implicit by-ref V%02d arg passed to call\n",
Expand Down