Skip to content

Commit

Permalink
Convert asserts in CEEInfo::getStaticFieldContent() to 'if' checks (#…
Browse files Browse the repository at this point in the history 
…100320)

* Add additional checks to optimization of constant static field loads

In `fgGetStaticFieldSeqAndAddress`, if we have a static field address
altered by a tree of `ADD CNS_INS` nodes, we need to verify that the
address is within the found field sequence. It might not be after
shared constant CSE kicks in (e.g., under OptRepeat), where the
sequence of ADDs might be the alter an arbitrary constant address from
one type into the address of the static field of a different type.
So we can't use the FieldSeq of the base address when considering
the full offset.

* Review feedback

1. Use `eeGetFieldName` / `eeGetClassName` return pointer
2. Only query extra metadata under `verbose || JitConfig.EnableExtraSuperPmiQueries()`

* Convert asserts in CEEInfo::getStaticFieldContent() to 'if' checks

* Make fgGetStaticFieldSeqAndAddress static

* Code review feedback
  • Loading branch information
@BruceForstall
BruceForstall authored Mar 27, 2024
1 parent 2a846ac commit c4796a3
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 56 deletions.
2 changes: 2 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5591,6 +5591,8 @@ class Compiler
void fgValueNumberFieldStore(
GenTree* storeNode, GenTree* baseAddr, FieldSeq* fieldSeq, ssize_t offset, unsigned storeSize, ValueNum value);

static bool fgGetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, ssize_t* byteOffset, FieldSeq** pFseq);

bool fgValueNumberConstLoad(GenTreeIndir* tree);

// Compute the value number for a byref-exposed load of the given type via the given pointerVN.
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/jit/fginline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,9 +590,9 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi

if (lcl->lvSingleDef)
{
bool isExact = false;
bool isNonNull = false;
CORINFO_CLASS_HANDLE newClass = m_compiler->gtGetClassHandle(value, &isExact, &isNonNull);
bool isExact;
bool isNonNull;
CORINFO_CLASS_HANDLE newClass = m_compiler->gtGetClassHandle(value, &isExact, &isNonNull);

if (newClass != NO_CLASS_HANDLE)
{
Expand Down
66 changes: 35 additions & 31 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14120,13 +14120,13 @@ GenTree* Compiler::gtFoldTypeCompare(GenTree* tree)
objOp = opOther->AsCall()->gtArgs.GetThisArg()->GetNode();
}

bool pIsExact = false;
bool pIsNonNull = false;
CORINFO_CLASS_HANDLE objCls = gtGetClassHandle(objOp, &pIsExact, &pIsNonNull);
bool isExact = false;
bool isNonNull = false;
CORINFO_CLASS_HANDLE objCls = gtGetClassHandle(objOp, &isExact, &isNonNull);

// if both classes are "final" (e.g. System.String[]) we can replace the comparison
// with `true/false` + null check.
if ((objCls != NO_CLASS_HANDLE) && (pIsExact || info.compCompHnd->isExactType(objCls)))
if ((objCls != NO_CLASS_HANDLE) && (isExact || info.compCompHnd->isExactType(objCls)))
{
TypeCompareState tcs = info.compCompHnd->compareTypesForEquality(objCls, clsHnd);
if (tcs != TypeCompareState::May)
Expand All @@ -14135,7 +14135,7 @@ GenTree* Compiler::gtFoldTypeCompare(GenTree* tree)
const bool typesAreEqual = tcs == TypeCompareState::Must;
GenTree* compareResult = gtNewIconNode((operatorIsEQ ^ typesAreEqual) ? 0 : 1);

if (!pIsNonNull)
if (!isNonNull)
{
// we still have to emit a null-check
// obj.GetType == typeof() -> (nullcheck) true/false
Expand Down Expand Up @@ -18394,24 +18394,23 @@ bool Compiler::gtStoreDefinesField(
// otherwise actual type may be a subtype.
// *pIsNonNull set true if tree value is known not to be null,
// otherwise a null value is possible.

//
CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, bool* pIsNonNull)
{
// Set default values for our out params.
*pIsNonNull = false;
*pIsExact = false;
CORINFO_CLASS_HANDLE objClass = nullptr;
*pIsNonNull = false;
*pIsExact = false;

// Bail out if the tree is not a ref type.
var_types treeType = tree->TypeGet();
if (treeType != TYP_REF)
if (!tree->TypeIs(TYP_REF))
{
return objClass;
return NO_CLASS_HANDLE;
}

// Tunnel through commas.
GenTree* obj = tree->gtEffectiveVal();
const genTreeOps objOp = obj->OperGet();
GenTree* obj = tree->gtEffectiveVal();
const genTreeOps objOp = obj->OperGet();
CORINFO_CLASS_HANDLE objClass = NO_CLASS_HANDLE;

switch (objOp)
{
Expand Down Expand Up @@ -18560,7 +18559,6 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b
assert(runtimeType != NO_CLASS_HANDLE);

objClass = runtimeType;
*pIsExact = false;
*pIsNonNull = true;
}

Expand Down Expand Up @@ -18604,9 +18602,6 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b
{
objClass = gtGetArrayElementClassHandle(base->AsArrElem()->gtArrObj);
}

*pIsExact = false;
*pIsNonNull = false;
}
else if (base->OperGet() == GT_ADD)
{
Expand Down Expand Up @@ -18643,7 +18638,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b
FieldSeq* fldSeq = base->AsIntCon()->gtFieldSeq;
if ((fldSeq != nullptr) && (fldSeq->GetOffset() == base->AsIntCon()->IconValue()))
{
CORINFO_FIELD_HANDLE fldHandle = base->AsIntCon()->gtFieldSeq->GetFieldHandle();
CORINFO_FIELD_HANDLE fldHandle = fldSeq->GetFieldHandle();
objClass = gtGetFieldClassHandle(fldHandle, pIsExact, pIsNonNull);
}
}
Expand Down Expand Up @@ -18710,7 +18705,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b
// Return Value:
// nullptr if helper call result is not a ref class, or the class handle
// is unknown, otherwise the class handle.

//
CORINFO_CLASS_HANDLE Compiler::gtGetHelperCallClassHandle(GenTreeCall* call, bool* pIsExact, bool* pIsNonNull)
{
assert(call->gtCallType == CT_HELPER);
Expand Down Expand Up @@ -18852,7 +18847,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetHelperCallClassHandle(GenTreeCall* call, boo
//
// Return Value:
// nullptr if element class handle is unknown, otherwise the class handle.

//
CORINFO_CLASS_HANDLE Compiler::gtGetArrayElementClassHandle(GenTree* array)
{
bool isArrayExact = false;
Expand Down Expand Up @@ -18894,9 +18889,12 @@ CORINFO_CLASS_HANDLE Compiler::gtGetArrayElementClassHandle(GenTree* array)
// is unknown, otherwise the class handle.
//
// May examine runtime state of static field instances.

//
CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldHnd, bool* pIsExact, bool* pIsNonNull)
{
*pIsExact = false;
*pIsNonNull = false;

CORINFO_CLASS_HANDLE fieldClass = NO_CLASS_HANDLE;
CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &fieldClass);

Expand All @@ -18909,11 +18907,14 @@ CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldH
if (queryForCurrentClass)
{
#if DEBUG
char fieldNameBuffer[128];
char classNameBuffer[128];
JITDUMP("\nQuerying runtime about current class of field %s (declared as %s)\n",
eeGetFieldName(fieldHnd, true, fieldNameBuffer, sizeof(fieldNameBuffer)),
eeGetClassName(fieldClass, classNameBuffer, sizeof(classNameBuffer)));
if (verbose || JitConfig.EnableExtraSuperPmiQueries())
{
char fieldNameBuffer[128];
char classNameBuffer[128];
const char* fieldName = eeGetFieldName(fieldHnd, true, fieldNameBuffer, sizeof(fieldNameBuffer));
const char* className = eeGetClassName(fieldClass, classNameBuffer, sizeof(classNameBuffer));
JITDUMP("\nQuerying runtime about current class of field %s (declared as %s)\n", fieldName, className);
}
#endif // DEBUG

// Is this a fully initialized init-only static field?
Expand All @@ -18928,10 +18929,13 @@ CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldH
*pIsExact = true;
*pIsNonNull = true;
#ifdef DEBUG
char buffer[128];
JITDUMP("Runtime reports field is init-only and initialized and has class %s\n",
eeGetClassName(fieldClass, buffer, sizeof(buffer)));
#endif
if (verbose || JitConfig.EnableExtraSuperPmiQueries())
{
char classNameBuffer2[128];
const char* className2 = eeGetClassName(fieldClass, classNameBuffer2, sizeof(classNameBuffer2));
JITDUMP("Runtime reports field is init-only and initialized and has class %s\n", className2);
}
#endif // DEBUG
}
else
{
Expand Down
11 changes: 8 additions & 3 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10919,7 +10919,11 @@ void Compiler::fgValueNumberSsaVarDef(GenTreeLclVarCommon* lcl)
// Return Value:
// true if the given tree is a static field address
//
static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, ssize_t* byteOffset, FieldSeq** pFseq)
/* static */
bool Compiler::fgGetStaticFieldSeqAndAddress(ValueNumStore* vnStore,
GenTree* tree,
ssize_t* byteOffset,
FieldSeq** pFseq)
{
VNFuncApp funcApp;
if (vnStore->GetVNFunc(tree->gtVNPair.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToStatic))
Expand All @@ -10934,7 +10938,6 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s
return true;
}
}
ssize_t val = 0;

// Special cases for NativeAOT:
// ADD(ICON_STATIC, CNS_INT) // nonGC-static base
Expand All @@ -10952,6 +10955,7 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s
}

// Accumulate final offset
ssize_t val = 0;
while (tree->OperIs(GT_ADD))
{
GenTree* op1 = tree->gtGetOp1();
Expand Down Expand Up @@ -10991,6 +10995,7 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s
return true;
}
}

return false;
}

Expand Down Expand Up @@ -11074,7 +11079,7 @@ bool Compiler::fgValueNumberConstLoad(GenTreeIndir* tree)
const int maxElementSize = sizeof(simd_t);

if (!tree->TypeIs(TYP_BYREF, TYP_STRUCT) &&
GetStaticFieldSeqAndAddress(vnStore, tree->gtGetOp1(), &byteOffset, &fieldSeq))
fgGetStaticFieldSeqAndAddress(vnStore, tree->gtGetOp1(), &byteOffset, &fieldSeq))
{
CORINFO_FIELD_HANDLE fieldHandle = fieldSeq->GetFieldHandle();
if ((fieldHandle != nullptr) && (size > 0) && (size <= maxElementSize) && ((size_t)byteOffset < INT_MAX))
Expand Down
31 changes: 18 additions & 13 deletions src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2336,12 +2336,16 @@ private bool getStaticFieldContent(CORINFO_FIELD_STRUCT_* fieldHandle, byte* buf

if (value == null)
{
Debug.Assert(valueOffset == 0);
Debug.Assert(bufferSize == targetPtrSize);

// Write "null" to buffer
new Span<byte>(buffer, targetPtrSize).Clear();
return true;
if ((valueOffset == 0) && (bufferSize == targetPtrSize))
{
// Write "null" to buffer
new Span<byte>(buffer, targetPtrSize).Clear();
return true;
}
else
{
return false;
}
}

if (value.GetRawData(_compilation.NodeFactory, out object data))
Expand All @@ -2357,13 +2361,14 @@ private bool getStaticFieldContent(CORINFO_FIELD_STRUCT_* fieldHandle, byte* buf
return false;

case FrozenObjectNode:
Debug.Assert(valueOffset == 0);
Debug.Assert(bufferSize == targetPtrSize);

// save handle's value to buffer
nint handle = ObjectToHandle(data);
new Span<byte>(&handle, targetPtrSize).CopyTo(new Span<byte>(buffer, targetPtrSize));
return true;
if ((valueOffset == 0) && (bufferSize == targetPtrSize))
{
// save handle's value to buffer
nint handle = ObjectToHandle(data);
new Span<byte>(&handle, targetPtrSize).CopyTo(new Span<byte>(buffer, targetPtrSize));
return true;
}
return false;
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/coreclr/vm/jitinterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11781,13 +11781,13 @@ bool CEEInfo::getStaticFieldContent(CORINFO_FIELD_HANDLE fieldHnd, uint8_t* buff
{
if (field->IsObjRef())
{
GCX_COOP();

_ASSERT(!field->IsRVA());
_ASSERT(valueOffset == 0); // there is no point in returning a chunk of a gc handle
_ASSERT((UINT)bufferSize == field->GetSize());
// there is no point in returning a chunk of a gc handle
if ((valueOffset == 0) && (sizeof(CORINFO_OBJECT_HANDLE) <= (UINT)bufferSize) && !field->IsRVA())
{
GCX_COOP();

result = getStaticObjRefContent(field->GetStaticOBJECTREF(), buffer, ignoreMovableObjects);
result = getStaticObjRefContent(field->GetStaticOBJECTREF(), buffer, ignoreMovableObjects);
}
}
else
{
Expand Down

0 comments on commit c4796a3

Please sign in to comment.