Skip to content

Commit

Permalink
Only use first field maps for shared statics (dotnet#66558)
Browse files Browse the repository at this point in the history 
* Use a handle for field sequences in VN

The field sequences are already canonical in the store,
so there is no need to have the somewhat involved code
in VN which essentially re-canonicalized them, we can
just use the pointer value (as a handle) to encode them.

Makes the future change of encoding some information in
the handle a little easier.

* Encode the statics

* Use shared-ness info in IsFieldAddr
  • Loading branch information
@SingleAccretion
SingleAccretion authored Mar 29, 2022
1 parent eaa1f05 commit a50d79f
Show file tree
Hide file tree
Showing 8 changed files with 238 additions and 241 deletions.
111 changes: 61 additions & 50 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10480,7 +10480,7 @@ void Compiler::gtDispFieldSeq(FieldSeqNode* pfsn)
while (pfsn != nullptr)
{
assert(pfsn != FieldSeqStore::NotAField()); // Can't exist in a field sequence list except alone
CORINFO_FIELD_HANDLE fldHnd = pfsn->m_fieldHnd;
CORINFO_FIELD_HANDLE fldHnd = pfsn->GetFieldHandleValue();
// First check the "pseudo" field handles...
if (fldHnd == FieldSeqStore::FirstElemPseudoField)
{
Expand All @@ -10494,7 +10494,7 @@ void Compiler::gtDispFieldSeq(FieldSeqNode* pfsn)
{
printf("%s", eeGetFieldName(fldHnd));
}
pfsn = pfsn->m_next;
pfsn = pfsn->GetNext();
if (pfsn != nullptr)
{
printf(", ");
Expand Down Expand Up @@ -16247,17 +16247,18 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
fldSeq = AsOp()->gtOp2->AsIntCon()->gtFieldSeq;
baseAddr = AsOp()->gtOp1;
}

if (baseAddr != nullptr)
else
{
assert(!baseAddr->TypeIs(TYP_REF) || !comp->GetZeroOffsetFieldMap()->Lookup(baseAddr));
return false;
}

assert(!baseAddr->TypeIs(TYP_REF) || !comp->GetZeroOffsetFieldMap()->Lookup(baseAddr));
}
else if (IsCnsIntOrI() && IsIconHandle(GTF_ICON_STATIC_HDL))
{
assert(!comp->GetZeroOffsetFieldMap()->Lookup(this) && (AsIntCon()->gtFieldSeq != nullptr));
fldSeq = AsIntCon()->gtFieldSeq;
baseAddr = nullptr;
baseAddr = this;
}
else if (comp->GetZeroOffsetFieldMap()->Lookup(this, &fldSeq))
{
Expand All @@ -16268,7 +16269,7 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
return false;
}

assert(fldSeq != nullptr);
assert((fldSeq != nullptr) && (baseAddr != nullptr));

if ((fldSeq == FieldSeqStore::NotAField()) || fldSeq->IsPseudoField())
{
Expand All @@ -16280,16 +16281,15 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
// or a static field. To avoid the expense of calling "getFieldClass" here, we will instead
// rely on the invariant that TYP_REF base addresses can never appear for struct fields - we
// will effectively treat such cases ("possible" in unsafe code) as undefined behavior.
if (comp->eeIsFieldStatic(fldSeq->GetFieldHandle()))
if (fldSeq->IsStaticField())
{
// TODO-VNTypes: this code is out of sync w.r.t. boxed statics that are numbered with
// VNF_PtrToStatic and treated as "simple" while here we treat them as "complex".
// For shared statics, we must encode the logical instantiation argument.
if (fldSeq->IsSharedStaticField())
{
*pBaseAddr = baseAddr;
}

// TODO-VNTypes: we will always return the "baseAddr" here for now, but strictly speaking,
// we only need to do that if we have a shared field, to encode the logical "instantiation"
// argument. In all other cases, this serves no purpose and just leads to redundant maps.
*pBaseAddr = baseAddr;
*pFldSeq = fldSeq;
*pFldSeq = fldSeq;
return true;
}

Expand Down Expand Up @@ -16657,17 +16657,12 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
}
if (fieldSeq != nullptr)
{
while (fieldSeq->m_next != nullptr)
{
fieldSeq = fieldSeq->m_next;
}
fieldSeq = fieldSeq->GetTail();

if (fieldSeq != FieldSeqStore::NotAField() && !fieldSeq->IsPseudoField())
{
CORINFO_FIELD_HANDLE fieldHnd = fieldSeq->m_fieldHnd;
CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &structHnd);
// With unsafe code and type casts
// this can return a primitive type and have nullptr for structHnd
// see runtime/issues/38541
// Note we may have a primitive here (and correctly fail to obtain the handle)
eeGetFieldType(fieldSeq->GetFieldHandle(), &structHnd);
}
}
}
Expand Down Expand Up @@ -16939,6 +16934,8 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b
}
else if (base->OperGet() == GT_ADD)
{
// TODO-VNTypes: use "IsFieldAddr" here instead.

// This could be a static field access.
//
// See if op1 is a static field base helper call
Expand All @@ -16954,20 +16951,15 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b

if (fieldSeq != nullptr)
{
while (fieldSeq->m_next != nullptr)
{
fieldSeq = fieldSeq->m_next;
}

assert(!fieldSeq->IsPseudoField());
fieldSeq = fieldSeq->GetTail();

// No benefit to calling gtGetFieldClassHandle here, as
// the exact field being accessed can vary.
CORINFO_FIELD_HANDLE fieldHnd = fieldSeq->m_fieldHnd;
CORINFO_CLASS_HANDLE fieldClass = nullptr;
CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &fieldClass);
CORINFO_FIELD_HANDLE fieldHnd = fieldSeq->GetFieldHandle();
CORINFO_CLASS_HANDLE fieldClass = NO_CLASS_HANDLE;
var_types fieldType = eeGetFieldType(fieldHnd, &fieldClass);

assert(fieldCorType == CORINFO_TYPE_CLASS);
assert(fieldType == TYP_REF);
objClass = fieldClass;
}
}
Expand Down Expand Up @@ -17321,18 +17313,18 @@ void GenTree::ParseArrayAddress(
noway_assert(!"fldSeqIter is NotAField() in ParseArrayAddress");
}

if (!FieldSeqStore::IsPseudoField(fldSeqIter->m_fieldHnd))
if (!FieldSeqStore::IsPseudoField(fldSeqIter->GetFieldHandleValue()))
{
if (*pFldSeq == nullptr)
{
*pFldSeq = fldSeqIter;
}
CORINFO_CLASS_HANDLE fldCls = nullptr;
noway_assert(fldSeqIter->m_fieldHnd != nullptr);
CorInfoType cit = comp->info.compCompHnd->getFieldType(fldSeqIter->m_fieldHnd, &fldCls);
noway_assert(fldSeqIter->GetFieldHandle() != NO_FIELD_HANDLE);
CorInfoType cit = comp->info.compCompHnd->getFieldType(fldSeqIter->GetFieldHandle(), &fldCls);
fieldOffsets += comp->compGetTypeSize(cit, fldCls);
}
fldSeqIter = fldSeqIter->m_next;
fldSeqIter = fldSeqIter->GetNext();
}

// Is there some portion of the "offset" beyond the first-elem offset and the struct field suffix we just computed?
Expand Down Expand Up @@ -17694,16 +17686,16 @@ void GenTree::LabelIndex(Compiler* comp, bool isConst)
// Note that the value of the below field doesn't matter; it exists only to provide a distinguished address.
//
// static
FieldSeqNode FieldSeqStore::s_notAField(nullptr, nullptr);
FieldSeqNode FieldSeqStore::s_notAField(nullptr, nullptr, FieldSeqNode::FieldKind::Instance);

// FieldSeqStore methods.
FieldSeqStore::FieldSeqStore(CompAllocator alloc) : m_alloc(alloc), m_canonMap(new (alloc) FieldSeqNodeCanonMap(alloc))
{
}

FieldSeqNode* FieldSeqStore::CreateSingleton(CORINFO_FIELD_HANDLE fieldHnd)
FieldSeqNode* FieldSeqStore::CreateSingleton(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode::FieldKind fieldKind)
{
FieldSeqNode fsn(fieldHnd, nullptr);
FieldSeqNode fsn(fieldHnd, nullptr, fieldKind);
FieldSeqNode* res = nullptr;
if (m_canonMap->Lookup(fsn, &res))
{
Expand Down Expand Up @@ -17738,8 +17730,8 @@ FieldSeqNode* FieldSeqStore::Append(FieldSeqNode* a, FieldSeqNode* b)
// Extremely special case for ConstantIndex pseudo-fields -- appending consecutive such
// together collapse to one.
}
else if (a->m_next == nullptr && a->m_fieldHnd == ConstantIndexPseudoField &&
b->m_fieldHnd == ConstantIndexPseudoField)
else if (a->GetNext() == nullptr && a->GetFieldHandleValue() == ConstantIndexPseudoField &&
b->GetFieldHandleValue() == ConstantIndexPseudoField)
{
return b;
}
Expand All @@ -17748,8 +17740,8 @@ FieldSeqNode* FieldSeqStore::Append(FieldSeqNode* a, FieldSeqNode* b)
// We should never add a duplicate FieldSeqNode
assert(a != b);

FieldSeqNode* tmp = Append(a->m_next, b);
FieldSeqNode fsn(a->m_fieldHnd, tmp);
FieldSeqNode* tmp = Append(a->GetNext(), b);
FieldSeqNode fsn(a->GetFieldHandleValue(), tmp, a->GetKind());
FieldSeqNode* res = nullptr;
if (m_canonMap->Lookup(fsn, &res))
{
Expand All @@ -17774,19 +17766,38 @@ CORINFO_FIELD_HANDLE FieldSeqStore::FirstElemPseudoField =
CORINFO_FIELD_HANDLE FieldSeqStore::ConstantIndexPseudoField =
(CORINFO_FIELD_HANDLE)&FieldSeqStore::ConstantIndexPseudoFieldStruct;

bool FieldSeqNode::IsFirstElemFieldSeq()
FieldSeqNode::FieldSeqNode(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode* next, FieldKind fieldKind) : m_next(next)
{
uintptr_t handleValue = reinterpret_cast<uintptr_t>(fieldHnd);

assert((handleValue & FIELD_KIND_MASK) == 0);
m_fieldHandleAndKind = handleValue | static_cast<uintptr_t>(fieldKind);

if (!FieldSeqStore::IsPseudoField(fieldHnd) && (fieldHnd != NO_FIELD_HANDLE))
{
assert(JitTls::GetCompiler()->eeIsFieldStatic(fieldHnd) == IsStaticField());
}
else
{
// Use the default for pseudo-fields.
assert(fieldKind == FieldKind::Instance);
}
}

bool FieldSeqNode::IsFirstElemFieldSeq() const
{
return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;
return GetFieldHandleValue() == FieldSeqStore::FirstElemPseudoField;
}

bool FieldSeqNode::IsConstantIndexFieldSeq()
bool FieldSeqNode::IsConstantIndexFieldSeq() const
{
return m_fieldHnd == FieldSeqStore::ConstantIndexPseudoField;
return GetFieldHandleValue() == FieldSeqStore::ConstantIndexPseudoField;
}

bool FieldSeqNode::IsPseudoField() const
{
return m_fieldHnd == FieldSeqStore::FirstElemPseudoField || m_fieldHnd == FieldSeqStore::ConstantIndexPseudoField;
return (GetFieldHandleValue() == FieldSeqStore::FirstElemPseudoField) ||
(GetFieldHandleValue() == FieldSeqStore::ConstantIndexPseudoField);
}

#ifdef FEATURE_SIMD
Expand Down
70 changes: 54 additions & 16 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,36 +220,71 @@ class AssertionInfo
}
};

/*****************************************************************************/

// GT_FIELD nodes will be lowered into more "code-gen-able" representations, like
// GT_IND's of addresses, or GT_LCL_FLD nodes. We'd like to preserve the more abstract
// information, and will therefore annotate such lowered nodes with FieldSeq's. A FieldSeq
// represents a (possibly) empty sequence of fields. The fields are in the order
// in which they are dereferenced. The first field may be an object field or a struct field;
// all subsequent fields must be struct fields.
struct FieldSeqNode
class FieldSeqNode
{
CORINFO_FIELD_HANDLE m_fieldHnd;
FieldSeqNode* m_next;
public:
enum class FieldKind : uintptr_t
{
Instance = 0, // An instance field, object or struct.
SimpleStatic = 1, // Simple static field - the handle represents a unique location.
SharedStatic = 2, // Static field on a shared generic type: "Class<__Canon>.StaticField".
};

private:
static const uintptr_t FIELD_KIND_MASK = 0b11;

static_assert_no_msg(sizeof(CORINFO_FIELD_HANDLE) == sizeof(uintptr_t));

uintptr_t m_fieldHandleAndKind;
FieldSeqNode* m_next;

public:
FieldSeqNode(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode* next, FieldKind fieldKind);

FieldKind GetKind() const
{
return static_cast<FieldKind>(m_fieldHandleAndKind & FIELD_KIND_MASK);
}

FieldSeqNode(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode* next) : m_fieldHnd(fieldHnd), m_next(next)
CORINFO_FIELD_HANDLE GetFieldHandle() const
{
assert(!IsPseudoField() && (GetFieldHandleValue() != NO_FIELD_HANDLE));
return GetFieldHandleValue();
}

CORINFO_FIELD_HANDLE GetFieldHandleValue() const
{
return CORINFO_FIELD_HANDLE(m_fieldHandleAndKind & ~FIELD_KIND_MASK);
}

// returns true when this is the pseudo #FirstElem field sequence
bool IsFirstElemFieldSeq();
bool IsFirstElemFieldSeq() const;

// returns true when this is the pseudo #ConstantIndex field sequence
bool IsConstantIndexFieldSeq();
bool IsConstantIndexFieldSeq() const;

// returns true when this is the the pseudo #FirstElem field sequence or the pseudo #ConstantIndex field sequence
bool IsPseudoField() const;

CORINFO_FIELD_HANDLE GetFieldHandle() const
bool IsStaticField() const
{
assert(!IsPseudoField() && (m_fieldHnd != nullptr));
return m_fieldHnd;
return (GetKind() == FieldKind::SimpleStatic) || (GetKind() == FieldKind::SharedStatic);
}

bool IsSharedStaticField() const
{
return GetKind() == FieldKind::SharedStatic;
}

FieldSeqNode* GetNext() const
{
return m_next;
}

FieldSeqNode* GetTail()
Expand All @@ -262,16 +297,17 @@ struct FieldSeqNode
return tail;
}

// Make sure this provides methods that allow it to be used as a KeyFuncs type in SimplerHash.
// Make sure this provides methods that allow it to be used as a KeyFuncs type in JitHashTable.
// Note that there is a one-to-one relationship between the field handle and the field kind, so
// we do not need to mask away the latter for comparison purposes.
static int GetHashCode(FieldSeqNode fsn)
{
return static_cast<int>(reinterpret_cast<intptr_t>(fsn.m_fieldHnd)) ^
static_cast<int>(reinterpret_cast<intptr_t>(fsn.m_next));
return static_cast<int>(fsn.m_fieldHandleAndKind) ^ static_cast<int>(reinterpret_cast<intptr_t>(fsn.m_next));
}

static bool Equals(const FieldSeqNode& fsn1, const FieldSeqNode& fsn2)
{
return fsn1.m_fieldHnd == fsn2.m_fieldHnd && fsn1.m_next == fsn2.m_next;
return fsn1.m_fieldHandleAndKind == fsn2.m_fieldHandleAndKind && fsn1.m_next == fsn2.m_next;
}
};

Expand All @@ -293,7 +329,8 @@ class FieldSeqStore
FieldSeqStore(CompAllocator alloc);

// Returns the (canonical in the store) singleton field sequence for the given handle.
FieldSeqNode* CreateSingleton(CORINFO_FIELD_HANDLE fieldHnd);
FieldSeqNode* CreateSingleton(CORINFO_FIELD_HANDLE fieldHnd,
FieldSeqNode::FieldKind fieldKind = FieldSeqNode::FieldKind::Instance);

// This is a special distinguished FieldSeqNode indicating that a constant does *not*
// represent a valid field sequence. This is "infectious", in the sense that appending it
Expand Down Expand Up @@ -556,6 +593,7 @@ enum GenTreeFlags : unsigned int
GTF_ICON_CIDMID_HDL = 0x0E000000, // GT_CNS_INT -- constant is a class ID or a module ID
GTF_ICON_BBC_PTR = 0x0F000000, // GT_CNS_INT -- constant is a basic block count pointer
GTF_ICON_STATIC_BOX_PTR = 0x10000000, // GT_CNS_INT -- constant is an address of the box for a STATIC_IN_HEAP field
GTF_ICON_FIELD_SEQ = 0x11000000, // <--------> -- constant is a FieldSeqNode* (used only as VNHandle)

// GTF_ICON_REUSE_REG_VAL = 0x00800000 // GT_CNS_INT -- GTF_REUSE_REG_VAL, defined above
GTF_ICON_FIELD_OFF = 0x00400000, // GT_CNS_INT -- constant is a field offset
Expand Down
Loading

0 comments on commit a50d79f

Please sign in to comment.