Skip to content

Commit

Permalink
JIT: Avoid unnecessary GTF_GLOB_REFs (#84349)
Browse files Browse the repository at this point in the history
* Avoid setting GTF_GLOB_REF on GT_FIELD_ADDR nodes
* Avoid setting GTF_GLOB_REF on GT_FIELD nodes off of implicit byrefs.
  This is ok now since implicit byref morphing indiscriminately sets
  GTF_GLOB_REF for these.
* Manually clone a "pointer to span" in span intrinsic expansion when it points to a local. Unfortunately this does not fall out from the above since gtClone does not handle FIELD_ADDR, and making it handle this needs some more work.

These changes are necessary to avoid address exposure in the two user
benchmarks in #83388.

Fix #74563
Fix #856
  • Loading branch information
jakobbotsch committed Apr 7, 2023
1 parent 092f752 commit 15c7022
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 28 deletions.
24 changes: 1 addition & 23 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7714,16 +7714,6 @@ GenTreeField* Compiler::gtNewFieldRef(var_types type, CORINFO_FIELD_HANDLE fldHn
LclVarDsc* varDsc = lvaGetDesc(obj->AsLclVarCommon());

varDsc->lvFieldAccessed = 1;

if (lvaIsImplicitByRefLocal(lvaGetLclNum(varDsc)))
{
// These structs are passed by reference and can easily become global references if those
// references are exposed. We clear out address-exposure information for these parameters
// when they are converted into references in fgRetypeImplicitByRefArgs() so we do not have
// the necessary information in morph to know if these indirections are actually global
// references, so we have to be conservative here.
fieldNode->gtFlags |= GTF_GLOB_REF;
}
}
else
{
Expand Down Expand Up @@ -7758,20 +7748,8 @@ GenTreeField* Compiler::gtNewFieldAddrNode(var_types type, CORINFO_FIELD_HANDLE
// If "obj" is the address of a local, note that a field of that struct local has been accessed.
if ((obj != nullptr) && obj->IsLclVarAddr())
{
LclVarDsc* varDsc = lvaGetDesc(obj->AsLclVarCommon());

LclVarDsc* varDsc = lvaGetDesc(obj->AsLclVarCommon());
varDsc->lvFieldAccessed = 1;

if (lvaIsImplicitByRefLocal(lvaGetLclNum(varDsc)))
{
// TODO-ADDR: delete this zero-diff quirk.
fieldNode->gtFlags |= GTF_GLOB_REF;
}
}
else
{
// TODO-ADDR: delete this zero-diff quirk.
fieldNode->gtFlags |= GTF_GLOB_REF;
}

// TODO-ADDR: add GTF_EXCEPT handling here and delete it from callers.
Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8397,9 +8397,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
}
else if (op1->TypeIs(TYP_BYREF, TYP_I_IMPL) && impIsAddressInLocal(op1))
{
// We mark implicit byrefs with GTF_GLOB_REF (see gtNewFieldRef for why).
// Avoid cloning for these.
clone = (op1->gtFlags & GTF_GLOB_REF) == 0;
clone = true;
}

if (clone)
Expand Down
12 changes: 10 additions & 2 deletions src/coreclr/jit/importercalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2881,8 +2881,16 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
// We need to use both index and ptr-to-span twice, so clone or spill.
index = impCloneExpr(index, &indexClone, NO_CLASS_HANDLE, CHECK_SPILL_ALL,
nullptr DEBUGARG("Span.get_Item index"));
ptrToSpan = impCloneExpr(ptrToSpan, &ptrToSpanClone, NO_CLASS_HANDLE, CHECK_SPILL_ALL,
nullptr DEBUGARG("Span.get_Item ptrToSpan"));

if (impIsAddressInLocal(ptrToSpan))
{
ptrToSpanClone = gtCloneExpr(ptrToSpan);
}
else
{
ptrToSpan = impCloneExpr(ptrToSpan, &ptrToSpanClone, NO_CLASS_HANDLE, CHECK_SPILL_ALL,
nullptr DEBUGARG("Span.get_Item ptrToSpan"));
}

// Bounds check
CORINFO_FIELD_HANDLE lengthHnd = info.compCompHnd->getFieldInClass(clsHnd, 1);
Expand Down

0 comments on commit 15c7022

Please sign in to comment.