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

Recognize more "local addr" trees as invariant #68484

Merged
merged 3 commits into from
Apr 25, 2022
Merged

Recognize more "local addr" trees as invariant #68484

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
72f7e02
Recognize more "local addr" trees as invariant
jakobbotsch Apr 25, 2022
20515dd
Switch to IsLocalAddrExpr()
jakobbotsch Apr 25, 2022
8bd6513
Run jit-format
jakobbotsch Apr 25, 2022
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
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4108,6 +4108,7 @@ class Compiler
static LONG jitNestingLevel;
#endif // DEBUG

static bool impIsInvariant(const GenTree* tree);
static bool impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut = nullptr);

void impMakeDiscretionaryInlineObservations(InlineInfo* pInlineInfo, InlineResult* inlineResult);
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16308,7 +16308,7 @@ bool GenTree::IsLocalExpr(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, Fie
// If this tree evaluates some sum of a local address and some constants,
// return the node for the local being addressed

GenTreeLclVarCommon* GenTree::IsLocalAddrExpr()
const GenTreeLclVarCommon* GenTree::IsLocalAddrExpr() const
{
if (OperGet() == GT_ADDR)
{
Expand Down Expand Up @@ -22865,7 +22865,7 @@ bool GenTreeLclFld::IsOffsetMisaligned() const

bool GenTree::IsInvariant() const
{
return OperIsConst() || Compiler::impIsAddressInLocal(this);
return OperIsConst() || IsLocalAddrExpr();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's notable that combined with #68469, this will bind IsLocalAddrExpr "the simple version" to LocalAddressVisitor like DefinesLocalAddr and IsLocalAddrExpr "the not simple version" are bound right now (which may also not be a new thing, it may have been like this already due to struct promotion requirements).

BTW, this is also the reason we must recognize ADDs in all these functions, they're produced for structs with fields at offsets larger than GenTreeLclFld (and the emitter) can represent.

Copy link
Member Author

@jakobbotsch jakobbotsch Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think even without #68469 there are/were non-trivial invariants between the two. While before this PR we 'supported' call morphing introducing a local store for the address, there was already an implicit assumption that no extra uses of this temp were ever created. So in a way we are just trading one assumption for another and have a slightly more explicit assert about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unsatisfying, but I am hard pressed to think about how to improve the situation.
Probably it would be ideal if we could unify DefinesLocalAddr, the two variants of IsLocalAddrExpr and what LocalAddressVisitor is doing somehow, but that also seems difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hehe, this is what #11057 is about :).

The original "north star" design was that we'd only have STORE_LCL_VAR|FLD for tracked locals, this return buffer optimization makes that "hard" unfortunately, but even with it (assuming I won't find a way to represent it as a store after all), we'd have:

  1. DefinesLocalAddr becomes equivalent to OperIsLocalAddr.
  2. All its variations (IsLocalAddrExpr "simple" and "not simple", as well as fgIsIndirOfAddrOfLocal) are deleted.
  3. LocalAddressVisitor transforms all ASGs into STORE_LCL_VAR|FLD, and for the return buffer optimization, LCL_VAR|FLD_ADDR.
  4. All indirect uses become LCL_VAR|FLD (this is Remove the OBJ(ADDR) wrapping created for calls by the importer #51569)

As a point-in-time improvement, the "not simple" IsLocalAddrExpr will be deleted once/if physical VN changes are in.

}

//------------------------------------------------------------------------
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1916,7 +1916,11 @@ struct GenTree

// Simpler variant of the above which just returns the local node if this is an expression that
// yields an address into a local
GenTreeLclVarCommon* IsLocalAddrExpr();
const GenTreeLclVarCommon* IsLocalAddrExpr() const;
GenTreeLclVarCommon* IsLocalAddrExpr()
{
return const_cast<GenTreeLclVarCommon*>(static_cast<const GenTree*>(this)->IsLocalAddrExpr());
}

// Determine if this tree represents the value of an entire implicit byref parameter,
// and if so return the tree for the parameter.
Expand Down
41 changes: 33 additions & 8 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18902,14 +18902,39 @@ bool Compiler::impIsValueType(typeInfo* pTypeInfo)
}
}

/*****************************************************************************
* Check to see if the tree is the address of a local or
the address of a field in a local.

*lclVarTreeOut will contain the GT_LCL_VAR tree when it returns true.

*/
//------------------------------------------------------------------------
// impIsInvariant: check if a tree (created during import) is invariant.
//
// Arguments:
// tree -- The tree
//
// Returns:
// true if it is invariant
//
// Remarks:
// This is a variant of GenTree::IsInvariant that is more suitable for use
// during import. Unlike that function, this one handles GT_FIELD nodes.
//
bool Compiler::impIsInvariant(const GenTree* tree)
{
return tree->OperIsConst() || impIsAddressInLocal(tree);
}

//------------------------------------------------------------------------
// impIsAddressInLocal:
// Check to see if the tree is the address of a local or
// the address of a field in a local.
// Arguments:
// tree -- The tree
// lclVarTreeOut -- [out] the local that this points into
//
// Returns:
// true if it points into a local
//
// Remarks:
// This is a variant of GenTree::IsLocalAddrExpr that is more suitable for
// use during import. Unlike that function, this one handles GT_FIELD nodes.
//
bool Compiler::impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut)
{
if (tree->gtOper != GT_ADDR)
Expand Down Expand Up @@ -19520,7 +19545,7 @@ void Compiler::impInlineRecordArgInfo(InlineInfo* pInlineInfo,
INDEBUG(curArgVal->AsLclVar()->gtLclILoffs = argNum;)
}

if (curArgVal->IsInvariant())
if (impIsInvariant(curArgVal))
{
inlCurArgInfo->argIsInvariant = true;
if (inlCurArgInfo->argIsThis && (curArgVal->gtOper == GT_CNS_INT) && (curArgVal->AsIntCon()->gtIconVal == 0))
Expand Down