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

JIT: enable tail calls and copy omission for implicit byref structs #33004

Merged
merged 9 commits into from
Mar 11, 2020
Merged
49 changes: 49 additions & 0 deletions src/coreclr/src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16193,6 +16193,55 @@ bool GenTree::IsLocalAddrExpr(Compiler* comp, GenTreeLclVarCommon** pLclVarTree,
return false;
}

//------------------------------------------------------------------------
// IsImplicitByrefParameterValue: determine if this tree is the entire
// value of a local implicit byref parameter
//
// Arguments:
// compiler -- compiler instance
//
// Return Value:
// GenTreeLclVarCommon node for the local, or nullptr.

GenTreeLclVarCommon* GenTree::IsImplicitByrefParameterValue(Compiler* compiler)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can change GenTreeLclVarCommon to GenTreeLclVar, it is more precise for this case.

{
#if defined(TARGET_AMD64) || defined(TARGET_ARM64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be limited to windows amd64?

Copy link
Member Author

Choose a reason for hiding this comment

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

Windows, yes.

We seem to think arm64 can have these too. I don't know the ABI well enough to say one way or another.

#if defined(TARGET_AMD64) || defined(TARGET_ARM64)
unsigned char lvIsImplicitByRef : 1; // Set if the argument is an implicit byref.
#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64)

Perhaps we need a define just for this and a cleanup of all references?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like windows arm64 uses the standard ARM64 EABI, so there are no implicit byrefs there. I'll put up a separate PR to clean this up here and throughout the rest of the jit codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that arm64 has implicit byrefs, but defining it for unix amd64 seems incorrect for sure.

Copy link
Contributor

@jashook jashook Mar 11, 2020

Choose a reason for hiding this comment

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

> 16 byte structs on arm64 windows or linux I believe are passed by reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see it now.. (from the EABI)

If the argument type is a Composite Type larger than 16 bytes, then the argument is copied to memory allocated by the caller, and the argument is replaced by a pointer to the copy.

So the right condition is windows x64, and all arm64.


GenTreeLclVarCommon* lcl = nullptr;

if (OperIs(GT_LCL_VAR))
{
lcl = AsLclVarCommon();
}
else if (OperIs(GT_OBJ))
{
GenTree* addr = AsIndir()->Addr();

if (addr->OperIs(GT_LCL_VAR))
{
lcl = addr->AsLclVarCommon();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this case, LCL_VAR is used as an address here (in other cases it is used as a value), so it has to be 'byref' or 'ref' type. Then how can it be lvaIsImplicitByRefLocal if it always should return false for TYP_IMPL size lclVars?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is potentially confusing.

The short answer is that before morph, arguments passed implicitly by-reference are represented by-value in the IR.
This gets changed in fgMorphImplicitByRefArgs where we make the by-reference aspect explicit. But the above helper gets called both before and after, and so needs to recognize cases where the local either represents a struct or a pointer to a struct.

Note the first two "match" cases here are pre-existing logic; all I did was move this to a helper method and add the last case.

}
else if (addr->OperIs(GT_ADDR))
{
Copy link
Member

Choose a reason for hiding this comment

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

Is there a semantic difference between OBJ(lcl) and OBJ(ADDR(lcl)) where lcl is an implicit byref parameter? Do they both represent a copy of the struct? When do we end up with one vs. the other?

Copy link
Member Author

Choose a reason for hiding this comment

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

What this means changes over time, and we're straddling that change.

See above?

GenTree* base = addr->AsOp()->gtOp1;

if (base->OperIs(GT_LCL_VAR))
{
lcl = base->AsLclVarCommon();
}
}
}

if ((lcl != nullptr) && compiler->lvaIsImplicitByRefLocal(lcl->GetLclNum()))
{
return lcl;
}

#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64)

return nullptr;
}

//------------------------------------------------------------------------
// IsLclVarUpdateTree: Determine whether this is an assignment tree of the
// form Vn = Vn 'oper' 'otherTree' where Vn is a lclVar
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/src/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -1841,6 +1841,10 @@ struct GenTree
// yields an address into a local
GenTreeLclVarCommon* IsLocalAddrExpr();

// Determine if this tree represents the value of an entire implict byref parameter,
// and if so return the tree for the parameter.
GenTreeLclVarCommon* IsImplicitByrefParameterValue(Compiler* compiler);

// Determine if this is a LclVarCommon node and return some additional info about it in the
// two out parameters.
bool IsLocalExpr(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, FieldSeqNode** pFldSeq);
Expand Down
61 changes: 59 additions & 2 deletions src/coreclr/src/jit/lclmorph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1097,10 +1097,67 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
{
return;
}

LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum);
JITDUMP("LocalAddressVisitor incrementing ref count from %d to %d for V%02d\n", varDsc->lvRefCnt(RCS_EARLY),
varDsc->lvRefCnt(RCS_EARLY) + 1, lclNum);
JITDUMP("LocalAddressVisitor incrementing ref count from %d to %d for implict by-ref V%02d\n",
varDsc->lvRefCnt(RCS_EARLY), varDsc->lvRefCnt(RCS_EARLY) + 1, lclNum);
varDsc->incLvRefCnt(1, RCS_EARLY);

// See if this struct is an argument to a call. This information is recorded
// via the weighted early ref count for the local, and feeds the undo promition
AndyAyersMS marked this conversation as resolved.
Show resolved Hide resolved
// heuristic.
//
// It can be approximate, so the pattern match below need not be exhaustive.
// But the pattern should at least subset the implicit byref cases that are
// handed in fgCanFastTailCall and fgMakeOutgoingStructArgCopy.
AndyAyersMS marked this conversation as resolved.
Show resolved Hide resolved
//
// CALL(OBJ(ADDR(LCL_VAR...)))
bool isArgToCall = false;
bool keepSearching = true;
for (int i = 0; i < m_ancestors.Height() && keepSearching; i++)
{
GenTree* node = m_ancestors.Top(i);
switch (i)
{
case 0:
{
keepSearching = node->OperIs(GT_LCL_VAR);
}
break;

case 1:
{
keepSearching = node->OperIs(GT_ADDR);
}
break;

case 2:
{
keepSearching = node->OperIs(GT_OBJ);
}
break;

case 3:
{
keepSearching = false;
isArgToCall = node->IsCall();
}
break;
default:
{
keepSearching = false;
}
break;
}
}

if (isArgToCall)
{
JITDUMP("LocalAddressVisitor incrementing weighted ref count from %d to %d"
" for implict by-ref V%02d arg passed to call\n",
varDsc->lvRefCntWtd(RCS_EARLY), varDsc->lvRefCntWtd(RCS_EARLY) + 1, lclNum);
varDsc->incLvRefCntWtd(1, RCS_EARLY);
}
}
};

Expand Down
Loading