From b8bd75ab4877d55e4408d79402a277d994fc1cbf Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 2 Dec 2021 15:29:58 +0300 Subject: [PATCH 1/7] DEBUG-ONLY: FieldInfo (2.0) Allows this change to be tested via SPMI. --- src/coreclr/jit/compiler.cpp | 11 +++- src/coreclr/jit/compiler.h | 70 +++++++++++++++++++--- src/coreclr/jit/ee_il_dll.hpp | 5 ++ src/coreclr/jit/gentree.cpp | 2 + src/coreclr/jit/gentree.h | 42 +++++++++++++ src/coreclr/jit/importer.cpp | 14 ++++- src/coreclr/jit/importer_vectorization.cpp | 3 + src/coreclr/jit/lclvars.cpp | 8 +++ src/coreclr/jit/morphblock.cpp | 6 ++ 9 files changed, 148 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 464933b88a8e7..4065465d95d6c 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -390,7 +390,7 @@ var_types Compiler::getJitGCType(BYTE gcType) // true if the given struct type contains only one pointer-sized integer value type, // false otherwise. // -bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) const +bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) { assert(info.compCompHnd->isValueClass(clsHnd)); if (info.compCompHnd->getClassSize(clsHnd) != TARGET_POINTER_SIZE) @@ -409,6 +409,8 @@ bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) const CORINFO_FIELD_HANDLE fldHnd = info.compCompHnd->getFieldInClass(clsHnd, 0); CorInfoType fieldType = info.compCompHnd->getFieldType(fldHnd, pClsHnd); + INDEBUG(RecordStructFieldInfo(fldHnd)); + var_types vt = JITtype2varType(fieldType); if (fieldType == CORINFO_TYPE_VALUECLASS) @@ -1914,8 +1916,11 @@ void Compiler::compInit(ArenaAllocator* pAlloc, m_nodeTestData = nullptr; m_loopHoistCSEClass = FIRST_LOOP_HOIST_CSE_CLASS; #endif - m_switchDescMap = nullptr; - m_blockToEHPreds = nullptr; + m_switchDescMap = nullptr; + m_blockToEHPreds = nullptr; +#ifdef DEBUG + m_fieldInfoMap = nullptr; +#endif // DEBUG m_fieldSeqStore = nullptr; m_zeroOffsetFieldMap = nullptr; m_arrayInfoMap = nullptr; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index dc8bb8f80cfa4..0ebf9e0d8e586 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2648,7 +2648,7 @@ class Compiler #endif // FEATURE_MULTIREG_RET #ifdef TARGET_X86 - bool isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) const; + bool isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd); #endif // TARGET_X86 //------------------------------------------------------------------------- @@ -11135,20 +11135,64 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX void JitTestCheckVN(); // Value numbering tests. #endif // DEBUG +#ifdef DEBUG + FieldInfoMap* m_fieldInfoMap; + + FieldInfoMap* GetFieldInfoMap() + { + Compiler* rootCompiler = impInlineRoot(); + if (rootCompiler->m_fieldInfoMap == nullptr) + { + CompAllocator allocator = rootCompiler->getAllocator(CMK_DebugOnly); + rootCompiler->m_fieldInfoMap = new (allocator) FieldInfoMap(allocator); + } + + return rootCompiler->m_fieldInfoMap; + } + + void RecordFieldInfo(CORINFO_FIELD_HANDLE fieldHnd, CORINFO_FIELD_INFO* eeFieldInfo) + { + FieldInfoMap* map = GetFieldInfoMap(); + + if (!map->Lookup(fieldHnd)) + { + map->Set(fieldHnd, new (map->GetAllocator()) FieldInfo(eeFieldInfo)); + } + } + + void RecordStructFieldInfo(CORINFO_FIELD_HANDLE fieldHnd) + { + FieldInfoMap* map = GetFieldInfoMap(); + + if (!map->Lookup(fieldHnd)) + { + map->Set(fieldHnd, new (map->GetAllocator()) FieldInfo()); + } + } + + const FieldInfo* GetFieldInfo(CORINFO_FIELD_HANDLE fieldHnd) + { + const FieldInfo* fieldInfo = nullptr; + bool fieldInfoFound = GetFieldInfoMap()->Lookup(fieldHnd, &fieldInfo); + assert(fieldInfoFound); + + return fieldInfo; + } +#endif // DEBUG + // The "FieldSeqStore", for canonicalizing field sequences. See the definition of FieldSeqStore for // operations. FieldSeqStore* m_fieldSeqStore; FieldSeqStore* GetFieldSeqStore() { - Compiler* compRoot = impInlineRoot(); - if (compRoot->m_fieldSeqStore == nullptr) + Compiler* rootCompiler = impInlineRoot(); + if (rootCompiler->m_fieldSeqStore == nullptr) { - // Create a CompAllocator that labels sub-structure with CMK_FieldSeqStore, and use that for allocation. - CompAllocator ialloc(getAllocator(CMK_FieldSeqStore)); - compRoot->m_fieldSeqStore = new (ialloc) FieldSeqStore(ialloc); + CompAllocator allocator = getAllocator(CMK_FieldSeqStore); + rootCompiler->m_fieldSeqStore = new (allocator) FieldSeqStore(allocator); } - return compRoot->m_fieldSeqStore; + return rootCompiler->m_fieldSeqStore; } typedef JitHashTable, FieldSeqNode*> NodeToFieldSeqMap; @@ -11262,7 +11306,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX { m_refAnyClass = info.compCompHnd->getBuiltinClass(CLASSID_TYPED_BYREF); } - return info.compCompHnd->getFieldInClass(m_refAnyClass, 0); + + CORINFO_FIELD_HANDLE dataFieldHnd = info.compCompHnd->getFieldInClass(m_refAnyClass, 0); + INDEBUG(RecordStructFieldInfo(dataFieldHnd)); + + return dataFieldHnd; } CORINFO_FIELD_HANDLE GetRefanyTypeField() { @@ -11270,7 +11318,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX { m_refAnyClass = info.compCompHnd->getBuiltinClass(CLASSID_TYPED_BYREF); } - return info.compCompHnd->getFieldInClass(m_refAnyClass, 1); + + CORINFO_FIELD_HANDLE typeFieldHnd = info.compCompHnd->getFieldInClass(m_refAnyClass, 1); + INDEBUG(RecordStructFieldInfo(typeFieldHnd)); + + return typeFieldHnd; } #if VARSET_COUNTOPS diff --git a/src/coreclr/jit/ee_il_dll.hpp b/src/coreclr/jit/ee_il_dll.hpp index 337ca8f147977..81433dd47d67e 100644 --- a/src/coreclr/jit/ee_il_dll.hpp +++ b/src/coreclr/jit/ee_il_dll.hpp @@ -42,6 +42,7 @@ void Compiler::eeGetFieldInfo(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_FIELD_INFO* pResult) { info.compCompHnd->getFieldInfo(pResolvedToken, info.compMethodHnd, accessFlags, pResult); + INDEBUG(RecordFieldInfo(pResolvedToken->hField, pResult)); } /***************************************************************************** @@ -64,7 +65,11 @@ bool Compiler::eeIsIntrinsic(CORINFO_METHOD_HANDLE ftn) FORCEINLINE bool Compiler::eeIsFieldStatic(CORINFO_FIELD_HANDLE fldHnd) { +#ifdef DEBUG + return GetFieldInfo(fldHnd)->IsStatic(); +#else return info.compCompHnd->isFieldStatic(fldHnd); +#endif } FORCEINLINE diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 8350e7ad725fb..d3cc7f5884a88 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -12737,6 +12737,8 @@ GenTree* Compiler::gtFoldBoxNullable(GenTree* tree) CORINFO_CLASS_HANDLE nullableHnd = gtGetStructHandle(arg->AsOp()->gtOp1); CORINFO_FIELD_HANDLE fieldHnd = info.compCompHnd->getFieldInClass(nullableHnd, 0); + INDEBUG(RecordStructFieldInfo(fieldHnd)); + // Replace the box with an access of the nullable 'hasValue' field. JITDUMP("\nSuccess: replacing BOX_NULLABLE(&x) [%06u] with x.hasValue\n", dspTreeID(op)); GenTree* newOp = gtNewFieldRef(TYP_BOOL, fieldHnd, arg, 0); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index d86d891b30327..0f3c0d08b7c7a 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -275,6 +275,48 @@ struct FieldSeqNode } }; +#ifdef DEBUG +// This class carries some about fields information neeeded for asserts from imporation to VN. +// Essentially, this is CORINFO_FIELD_INFO but Jit-specific and smaller. +class FieldInfo +{ + const unsigned m_flags; + const CORINFO_FIELD_ACCESSOR m_accessor; + +public: + FieldInfo(CORINFO_FIELD_INFO* eeFieldInfo) + : m_flags(eeFieldInfo->fieldFlags), m_accessor(eeFieldInfo->fieldAccessor) + { + } + + FieldInfo() : m_flags(0), m_accessor(CORINFO_FIELD_INSTANCE) + { + } + + bool IsStatic() const + { + return (m_flags & CORINFO_FLG_FIELD_STATIC) != 0; + } + + bool IsBoxedStatic() const + { + return (m_flags & CORINFO_FLG_FIELD_STATIC_IN_HEAP) != 0; + } + + bool IsSharedStatic() const + { + return m_accessor == CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER; + } + + bool IsReadOnly() const + { + return (m_flags & CORINFO_FLG_FIELD_FINAL) != 0; + } +}; + +typedef JitHashTable, const FieldInfo*> FieldInfoMap; +#endif // DEBUG + // This class canonicalizes field sequences. class FieldSeqStore { diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 54ad01eaf928d..792d1cb3d24dd 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3712,6 +3712,9 @@ GenTree* Compiler::impCreateSpanIntrinsic(CORINFO_SIG_INFO* sig) CORINFO_FIELD_HANDLE pointerFieldHnd = info.compCompHnd->getFieldInClass(spanHnd, 0); CORINFO_FIELD_HANDLE lengthFieldHnd = info.compCompHnd->getFieldInClass(spanHnd, 1); + INDEBUG(RecordStructFieldInfo(pointerFieldHnd)); + INDEBUG(RecordStructFieldInfo(lengthFieldHnd)); + GenTreeLclFld* pointerField = gtNewLclFldNode(spanTempNum, TYP_BYREF, 0); pointerField->SetFieldSeq(GetFieldSeqStore()->CreateSingleton(pointerFieldHnd)); GenTree* pointerFieldAsg = gtNewAssignNode(pointerField, pointerValue); @@ -3994,6 +3997,8 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, assert(byReferenceStruct != nullptr); impPushOnStack(byReferenceStruct, typeInfo(TI_STRUCT, clsHnd)); retNode = assign; + + INDEBUG(RecordStructFieldInfo(fldHnd)); break; } @@ -4004,6 +4009,8 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, CORINFO_FIELD_HANDLE fldHnd = info.compCompHnd->getFieldInClass(clsHnd, 0); GenTree* field = gtNewFieldRef(TYP_BYREF, fldHnd, op1, 0); retNode = field; + + INDEBUG(RecordStructFieldInfo(fldHnd)); break; } @@ -4128,6 +4135,8 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, GenTree* length = gtNewFieldRef(TYP_INT, lengthHnd, ptrToSpan, lengthOffset); GenTree* boundsCheck = new (this, GT_BOUNDS_CHECK) GenTreeBoundsChk(index, length, SCK_RNGCHK_FAIL); + INDEBUG(RecordStructFieldInfo(lengthHnd)); + // Element access index = indexClone; @@ -4153,6 +4162,8 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, GenTree* data = gtNewFieldRef(TYP_BYREF, ptrHnd, ptrToSpanClone, ptrOffset); GenTree* result = gtNewOperNode(GT_ADD, TYP_BYREF, data, index); + INDEBUG(RecordStructFieldInfo(ptrHnd)); + // Prepare result var_types resultType = JITtype2varType(sig->retType); assert(resultType == result->TypeGet()); @@ -7110,6 +7121,8 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, const CORINFO_FIELD_HANDLE hasValueFldHnd = info.compCompHnd->getFieldInClass(nullableCls, 0); + INDEBUG(RecordStructFieldInfo(hasValueFldHnd)); + assert(info.compCompHnd->getFieldOffset(hasValueFldHnd) == 0); assert(!strcmp(info.compCompHnd->getFieldName(hasValueFldHnd, nullptr), "hasValue")); @@ -8231,7 +8244,6 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT !isBoxedStatic ? GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField) : FieldSeqStore::NotAField(); GenTree* op1; - switch (pFieldInfo->fieldAccessor) { case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER: diff --git a/src/coreclr/jit/importer_vectorization.cpp b/src/coreclr/jit/importer_vectorization.cpp index 00a5389a8939e..192fa4e68a113 100644 --- a/src/coreclr/jit/importer_vectorization.cpp +++ b/src/coreclr/jit/importer_vectorization.cpp @@ -698,6 +698,9 @@ GenTree* Compiler::impSpanEqualsOrStartsWith(bool startsWith, CORINFO_SIG_INFO* CORINFO_FIELD_HANDLE lengthHnd = info.compCompHnd->getFieldInClass(spanCls, 1); const unsigned lengthOffset = info.compCompHnd->getFieldOffset(lengthHnd); + INDEBUG(RecordStructFieldInfo(pointerHnd)); + INDEBUG(RecordStructFieldInfo(lengthHnd)); + // Create a placeholder for Span object - we're not going to Append it to statements // in advance to avoid redundant spills in case if we fail to vectorize unsigned spanObjRef = lvaGrabTemp(true DEBUGARG("spanObj tmp")); diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index df32fcf45dc85..28468efd8803a 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -1730,6 +1730,8 @@ bool Compiler::StructPromotionHelper::CanPromoteStructType(CORINFO_CLASS_HANDLE structPromotionInfo.fields[ordinal] = GetFieldInfo(fieldHnd, ordinal); const lvaStructFieldInfo& fieldInfo = structPromotionInfo.fields[ordinal]; + INDEBUG(compiler->RecordStructFieldInfo(fieldHnd)); + noway_assert(fieldInfo.fldOffset < structSize); if (fieldInfo.fldSize == 0) @@ -2192,6 +2194,9 @@ bool Compiler::StructPromotionHelper::TryPromoteStructField(lvaStructFieldInfo& // the struct field. CORINFO_FIELD_HANDLE innerFieldHndl = compHandle->getFieldInClass(fieldInfo.fldTypeHnd, 0); unsigned innerFieldOffset = compHandle->getFieldOffset(innerFieldHndl); + + INDEBUG(compiler->RecordStructFieldInfo(innerFieldHndl)); + if (innerFieldOffset != 0) { return false; @@ -2833,6 +2838,9 @@ void Compiler::makeExtraStructQueries(CORINFO_CLASS_HANDLE structHandle, int lev CORINFO_CLASS_HANDLE fieldClassHandle = NO_CLASS_HANDLE; CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHandle, &fieldClassHandle); var_types fieldVarType = JITtype2varType(fieldCorType); + + INDEBUG(RecordStructFieldInfo(fieldHandle)); + if (fieldClassHandle != NO_CLASS_HANDLE) { if (varTypeIsStruct(fieldVarType)) diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index b8c12020b306f..000137c8e8bbe 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -1237,6 +1237,9 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() CORINFO_CLASS_HANDLE classHnd = srcVarDsc->GetStructHnd(); CORINFO_FIELD_HANDLE fieldHnd = m_comp->info.compCompHnd->getFieldInClass(classHnd, srcFieldVarDsc->lvFldOrdinal); + + INDEBUG(m_comp->RecordStructFieldInfo(fieldHnd)); + FieldSeqNode* curFieldSeq = m_comp->GetFieldSeqStore()->CreateSingleton(fieldHnd); unsigned srcFieldOffset = m_comp->lvaGetDesc(srcFieldLclNum)->lvFldOffset; @@ -1342,6 +1345,9 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() CORINFO_FIELD_HANDLE fieldHnd = m_comp->info.compCompHnd->getFieldInClass(classHnd, m_comp->lvaGetDesc(dstFieldLclNum)->lvFldOrdinal); + + INDEBUG(m_comp->RecordStructFieldInfo(fieldHnd)); + FieldSeqNode* curFieldSeq = m_comp->GetFieldSeqStore()->CreateSingleton(fieldHnd); var_types destType = m_comp->lvaGetDesc(dstFieldLclNum)->lvType; From 2036937e9d0cee3875c8382d847d58df388a6f94 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 13 Mar 2022 00:49:03 +0300 Subject: [PATCH 2/7] 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. --- src/coreclr/jit/gentree.h | 1 + src/coreclr/jit/valuenum.cpp | 177 ++++++++++++-------------------- src/coreclr/jit/valuenum.h | 13 +-- src/coreclr/jit/valuenumfuncs.h | 3 - 4 files changed, 68 insertions(+), 126 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 0f3c0d08b7c7a..06f642e294683 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -598,6 +598,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 diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index e52b3226f3bd8..1005c50dfd415 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -1962,7 +1962,6 @@ class Object* ValueNumStore::s_specialRefConsts[] = {nullptr, nullptr, nullptr}; ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func) { assert(VNFuncArity(func) == 0); - assert(func != VNF_NotAField); ValueNum resultVN; @@ -4156,11 +4155,36 @@ ValueNumPair ValueNumStore::VNPairApplySelectors(ValueNumPair map, FieldSeqNode* return ValueNumPair(liberalVN, conservVN); } +//------------------------------------------------------------------------ +// IsVNNotAField: Is the value number a "NotAField" field sequence? +// +// Arguments: +// vn - the value number in question +// +// Return Value: +// Whether "vn" represents a "NotAField" field sequence. +// +// Notes: +// "NotAField" field sequences always get a "new, unique" number, since +// they represent unknown locations. Thus they get their own chunk kind, +// for which no actual memory is allocated to hold VNFunc data. +// bool ValueNumStore::IsVNNotAField(ValueNum vn) { return m_chunks.GetNoExpand(GetChunkNum(vn))->m_attribs == CEA_NotAField; } +//------------------------------------------------------------------------ +// VNForFieldSeq: Get the value number representing a field sequence. +// +// Arguments: +// fieldSeq - the field sequence +// +// Return Value: +// "VNForNull" if the sequence is empty ("nullptr"). +// "IsVNNotAField" VN if is a "NotAField" +// "GTF_FIELD_SEQ_PTR" handle VN otherwise. +// ValueNum ValueNumStore::VNForFieldSeq(FieldSeqNode* fieldSeq) { if (fieldSeq == nullptr) @@ -4178,16 +4202,14 @@ ValueNum ValueNumStore::VNForFieldSeq(FieldSeqNode* fieldSeq) } else { - ssize_t fieldHndVal = ssize_t(fieldSeq->m_fieldHnd); - ValueNum fieldHndVN = VNForHandle(fieldHndVal, GTF_ICON_FIELD_HDL); - ValueNum seqNextVN = VNForFieldSeq(fieldSeq->m_next); - fieldSeqVN = VNForFunc(TYP_REF, VNF_FieldSeq, fieldHndVN, seqNextVN); + // This encoding relies on the canonicality of field sequences. + fieldSeqVN = VNForHandle(reinterpret_cast(fieldSeq), GTF_ICON_FIELD_SEQ); } #ifdef DEBUG if (m_pComp->verbose) { - printf(" FieldSeq"); + printf(" "); vnDump(m_pComp, fieldSeqVN); printf(" is " FMT_VN "\n", fieldSeqVN); } @@ -4196,6 +4218,15 @@ ValueNum ValueNumStore::VNForFieldSeq(FieldSeqNode* fieldSeq) return fieldSeqVN; } +//------------------------------------------------------------------------ +// FieldSeqVNToFieldSeq: Decode the field sequence from a VN representing one. +// +// Arguments: +// vn - the value number, must be one obtained using "VNForFieldSeq" +// +// Return Value: +// The field sequence associated with "vn". +// FieldSeqNode* ValueNumStore::FieldSeqVNToFieldSeq(ValueNum vn) { if (vn == VNForNull()) @@ -4203,54 +4234,32 @@ FieldSeqNode* ValueNumStore::FieldSeqVNToFieldSeq(ValueNum vn) return nullptr; } - assert(IsVNFunc(vn)); - - VNFuncApp funcApp; - GetVNFunc(vn, &funcApp); - if (funcApp.m_func == VNF_NotAField) + if (IsVNNotAField(vn)) { return FieldSeqStore::NotAField(); } - assert(funcApp.m_func == VNF_FieldSeq); - const ssize_t fieldHndVal = ConstantValue(funcApp.m_args[0]); - FieldSeqNode* head = - m_pComp->GetFieldSeqStore()->CreateSingleton(reinterpret_cast(fieldHndVal)); - FieldSeqNode* tail = FieldSeqVNToFieldSeq(funcApp.m_args[1]); - return m_pComp->GetFieldSeqStore()->Append(head, tail); + assert(IsVNHandle(vn) && (GetHandleFlags(vn) == GTF_ICON_FIELD_SEQ)); + + return reinterpret_cast(ConstantValue(vn)); } -ValueNum ValueNumStore::FieldSeqVNAppend(ValueNum fsVN1, ValueNum fsVN2) +//------------------------------------------------------------------------ +// FieldSeqVNAppend: Append a field sequences to one represented as a VN. +// +// Arguments: +// innerFieldSeqVN - VN of the field sequence being appended to +// outerFieldSeq - the field sequence being appended +// +// Return Value: +// The value number representing [innerFieldSeq, outerFieldSeq]. +// +ValueNum ValueNumStore::FieldSeqVNAppend(ValueNum innerFieldSeqVN, FieldSeqNode* outerFieldSeq) { - if (fsVN1 == VNForNull()) - { - return fsVN2; - } - - assert(IsVNFunc(fsVN1)); - - VNFuncApp funcApp1; - GetVNFunc(fsVN1, &funcApp1); + FieldSeqNode* innerFieldSeq = FieldSeqVNToFieldSeq(innerFieldSeqVN); + FieldSeqNode* fullFieldSeq = m_pComp->GetFieldSeqStore()->Append(innerFieldSeq, outerFieldSeq); - if ((funcApp1.m_func == VNF_NotAField) || IsVNNotAField(fsVN2)) - { - return VNForFieldSeq(FieldSeqStore::NotAField()); - } - - assert(funcApp1.m_func == VNF_FieldSeq); - ValueNum tailRes = FieldSeqVNAppend(funcApp1.m_args[1], fsVN2); - ValueNum fieldSeqVN = VNForFunc(TYP_REF, VNF_FieldSeq, funcApp1.m_args[0], tailRes); - -#ifdef DEBUG - if (m_pComp->verbose) - { - printf(" fieldSeq " FMT_VN " is ", fieldSeqVN); - vnDump(m_pComp, fieldSeqVN); - printf("\n"); - } -#endif - - return fieldSeqVN; + return VNForFieldSeq(fullFieldSeq); } ValueNum ValueNumStore::ExtendPtrVN(GenTree* opA, GenTree* opB) @@ -4292,24 +4301,22 @@ ValueNum ValueNumStore::ExtendPtrVN(GenTree* opA, FieldSeqNode* fldSeq) VNFuncApp consFuncApp; assert(GetVNFunc(VNConservativeNormalValue(opA->gtVNPair), &consFuncApp) && consFuncApp.Equals(funcApp)); #endif - ValueNum fldSeqVN = VNForFieldSeq(fldSeq); - res = VNForFunc(TYP_BYREF, VNF_PtrToLoc, funcApp.m_args[0], FieldSeqVNAppend(funcApp.m_args[1], fldSeqVN)); + res = VNForFunc(TYP_BYREF, VNF_PtrToLoc, funcApp.m_args[0], FieldSeqVNAppend(funcApp.m_args[1], fldSeq)); } else if (funcApp.m_func == VNF_PtrToStatic) { - ValueNum fldSeqVN = VNForFieldSeq(fldSeq); - res = VNForFunc(TYP_BYREF, VNF_PtrToStatic, funcApp.m_args[0], FieldSeqVNAppend(funcApp.m_args[1], fldSeqVN)); + res = VNForFunc(TYP_BYREF, VNF_PtrToStatic, funcApp.m_args[0], FieldSeqVNAppend(funcApp.m_args[1], fldSeq)); } else if (funcApp.m_func == VNF_PtrToArrElem) { - ValueNum fldSeqVN = VNForFieldSeq(fldSeq); res = VNForFunc(TYP_BYREF, VNF_PtrToArrElem, funcApp.m_args[0], funcApp.m_args[1], funcApp.m_args[2], - FieldSeqVNAppend(funcApp.m_args[3], fldSeqVN)); + FieldSeqVNAppend(funcApp.m_args[3], fldSeq)); } if (res != NoVN) { res = VNWithExc(res, opAvnx); } + return res; } @@ -5888,7 +5895,6 @@ bool ValueNumStore::IsVNFunc(ValueNum vn) Chunk* c = m_chunks.GetNoExpand(GetChunkNum(vn)); switch (c->m_attribs) { - case CEA_NotAField: case CEA_Func0: case CEA_Func1: case CEA_Func2: @@ -5957,12 +5963,6 @@ bool ValueNumStore::GetVNFunc(ValueNum vn, VNFuncApp* funcApp) funcApp->m_arity = 0; return true; } - case CEA_NotAField: - { - funcApp->m_func = VNF_NotAField; - funcApp->m_arity = 0; - return true; - } default: return false; } @@ -6021,6 +6021,11 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) { printf("NoVN"); } + else if (IsVNNotAField(vn) || (IsVNHandle(vn) && (GetHandleFlags(vn) == GTF_ICON_FIELD_SEQ))) + { + comp->gtDispAnyFieldSeq(FieldSeqVNToFieldSeq(vn)); + printf(" "); + } else if (IsVNHandle(vn)) { ssize_t val = ConstantValue(vn); @@ -6146,10 +6151,6 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) // A few special cases... switch (funcApp.m_func) { - case VNF_FieldSeq: - case VNF_NotAField: - vnDumpFieldSeq(comp, &funcApp, true); - break; case VNF_MapSelect: vnDumpMapSelect(comp, &funcApp); break; @@ -6253,56 +6254,6 @@ void ValueNumStore::vnDumpExcSeq(Compiler* comp, VNFuncApp* excSeq, bool isHead) } } -void ValueNumStore::vnDumpFieldSeq(Compiler* comp, VNFuncApp* fieldSeq, bool isHead) -{ - if (fieldSeq->m_func == VNF_NotAField) - { - printf(""); - return; - } - - assert(fieldSeq->m_func == VNF_FieldSeq); // Precondition. - // First arg is the field handle VN. - assert(IsVNConstant(fieldSeq->m_args[0]) && TypeOfVN(fieldSeq->m_args[0]) == TYP_I_IMPL); - ssize_t fieldHndVal = ConstantValue(fieldSeq->m_args[0]); - bool hasTail = (fieldSeq->m_args[1] != VNForNull()); - - if (isHead && hasTail) - { - printf("("); - } - - CORINFO_FIELD_HANDLE fldHnd = CORINFO_FIELD_HANDLE(fieldHndVal); - if (fldHnd == FieldSeqStore::FirstElemPseudoField) - { - printf("#FirstElem"); - } - else if (fldHnd == FieldSeqStore::ConstantIndexPseudoField) - { - printf("#ConstantIndex"); - } - else - { - const char* modName; - const char* fldName = m_pComp->eeGetFieldName(fldHnd, &modName); - printf("%s", fldName); - } - - if (hasTail) - { - printf(", "); - assert(IsVNFunc(fieldSeq->m_args[1])); - VNFuncApp tail; - GetVNFunc(fieldSeq->m_args[1], &tail); - vnDumpFieldSeq(comp, &tail, false); - } - - if (isHead && hasTail) - { - printf(")"); - } -} - void ValueNumStore::vnDumpMapSelect(Compiler* comp, VNFuncApp* mapSelect) { assert(mapSelect->m_func == VNF_MapSelect); // Precondition. diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index f15dd5cc87624..b76a08d33adae 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -685,20 +685,13 @@ class ValueNumStore bool srcIsUnsigned = false, bool hasOverflowCheck = false); - // Returns true iff the VN represents an application of VNF_NotAField. bool IsVNNotAField(ValueNum vn); - // PtrToLoc values need to express a field sequence as one of their arguments. VN for null represents - // empty sequence, otherwise, "FieldSeq(VN(FieldHandle), restOfSeq)". ValueNum VNForFieldSeq(FieldSeqNode* fieldSeq); - // Requires that "vn" represents a field sequence, that is, is the result of a call to VNForFieldSeq. - // Returns the FieldSequence it represents. FieldSeqNode* FieldSeqVNToFieldSeq(ValueNum vn); - // Both argument must represent field sequences; returns the value number representing the - // concatenation "fsVN1 || fsVN2". - ValueNum FieldSeqVNAppend(ValueNum fsVN1, ValueNum fsVN2); + ValueNum FieldSeqVNAppend(ValueNum innerFieldSeqVN, FieldSeqNode* outerFieldSeq); // If "opA" has a PtrToLoc, PtrToArrElem, or PtrToStatic application as its value numbers, and "opB" is an integer // with a "fieldSeq", returns the VN for the pointer form extended with the field sequence; or else NoVN. @@ -1020,9 +1013,9 @@ class ValueNumStore // Prints, to standard out, a representation of "vn". void vnDump(Compiler* comp, ValueNum vn, bool isPtr = false); - // Requires "fieldSeq" to be a field sequence VNFuncApp. + // Requires "fieldSeq" to be a field sequence VN. // Prints a representation (comma-separated list of field names) on standard out. - void vnDumpFieldSeq(Compiler* comp, VNFuncApp* fieldSeq, bool isHead); + void vnDumpFieldSeq(Compiler* comp, ValueNum fieldSeqVN); // Requires "mapSelect" to be a map select VNFuncApp. // Prints a representation of a MapSelect operation on standard out. diff --git a/src/coreclr/jit/valuenumfuncs.h b/src/coreclr/jit/valuenumfuncs.h index 74e8b90ff19bb..cc88c400a5a33 100644 --- a/src/coreclr/jit/valuenumfuncs.h +++ b/src/coreclr/jit/valuenumfuncs.h @@ -10,9 +10,6 @@ ValueNumFuncDef(MemOpaque, 1, false, false, false) // Args: 0: loop num ValueNumFuncDef(MapStore, 4, false, false, false) // Args: 0: map, 1: index (e. g. field handle), 2: value being stored, 3: loop num. ValueNumFuncDef(MapSelect, 2, false, false, false) // Args: 0: map, 1: key. -ValueNumFuncDef(FieldSeq, 2, false, false, false) // Sequence (VN of null == empty) of (VN's of) field handles. -ValueNumFuncDef(NotAField, 0, false, false, false) // Value number function for FieldSeqStore::NotAField. - ValueNumFuncDef(PtrToLoc, 2, false, true, false) // Pointer (byref) to a local variable. Args: VN's of: 0: var num, 1: FieldSeq. ValueNumFuncDef(PtrToArrElem, 4, false, false, false) // Pointer (byref) to an array element. Args: 0: array elem type eq class var_types value, VN's of: 1: array, 2: index, 3: FieldSeq. ValueNumFuncDef(PtrToStatic, 2, false, true, false) // Pointer (byref) to a static variable (or possibly a field thereof, if the static variable is a struct). From d78929b3394b54bc357b7e51ca00eb6fcc2a477f Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Wed, 9 Mar 2022 14:54:03 +0300 Subject: [PATCH 3/7] Encode the statics --- src/coreclr/jit/gentree.cpp | 85 ++++++++++++++++++++---------------- src/coreclr/jit/gentree.h | 69 ++++++++++++++++++++++------- src/coreclr/jit/importer.cpp | 20 +++++---- src/coreclr/jit/lclmorph.cpp | 3 +- src/coreclr/jit/morph.cpp | 63 +++++++++++++------------- src/coreclr/jit/valuenum.cpp | 21 ++++----- 6 files changed, 158 insertions(+), 103 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d3cc7f5884a88..b02659419b379 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -10382,7 +10382,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) { @@ -10396,7 +10396,7 @@ void Compiler::gtDispFieldSeq(FieldSeqNode* pfsn) { printf("%s", eeGetFieldName(fldHnd)); } - pfsn = pfsn->m_next; + pfsn = pfsn->GetNext(); if (pfsn != nullptr) { printf(", "); @@ -16550,17 +16550,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); } } } @@ -16832,6 +16827,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 @@ -16847,20 +16844,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; } } @@ -17214,18 +17206,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->GetNext() != nullptr); + 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? @@ -17587,16 +17579,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)) { @@ -17631,8 +17623,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; } @@ -17641,8 +17633,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)) { @@ -17667,19 +17659,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(fieldHnd); + + assert((handleValue & FIELD_KIND_MASK) == 0); + m_fieldHandleAndKind = handleValue | static_cast(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 diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 06f642e294683..d73e7709d5a88 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -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(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() @@ -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(reinterpret_cast(fsn.m_fieldHnd)) ^ - static_cast(reinterpret_cast(fsn.m_next)); + return static_cast(fsn.m_fieldHandleAndKind) ^ static_cast(reinterpret_cast(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; } }; @@ -335,7 +371,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 diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 792d1cb3d24dd..87d08dc53440f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -8233,15 +8233,18 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT // be mutable, but the only current producer of such images, the C++/CLI compiler, does // not appear to support mapping different fields to the same address. So we will say // that "mutable overlapping RVA statics" are UB as well. If this ever changes, code in - // morph and value numbering will need to be updated to respect "gtFldMayOverlap" and - // "NotAField FldSeq". + // value numbering will need to be updated to respect "NotAField FldSeq". // For statics that are not "boxed", the initial address tree will contain the field sequence. // For those that are, we will attach it later, when adding the indirection for the box, since // that tree will represent the true address. - bool isBoxedStatic = (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_STATIC_IN_HEAP) != 0; - FieldSeqNode* innerFldSeq = - !isBoxedStatic ? GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField) : FieldSeqStore::NotAField(); + bool isBoxedStatic = (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_STATIC_IN_HEAP) != 0; + bool isSharedStatic = (pFieldInfo->fieldAccessor == CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER) || + (pFieldInfo->fieldAccessor == CORINFO_FIELD_STATIC_READYTORUN_HELPER); + FieldSeqNode::FieldKind fieldKind = + isSharedStatic ? FieldSeqNode::FieldKind::SharedStatic : FieldSeqNode::FieldKind::SimpleStatic; + FieldSeqNode* innerFldSeq = !isBoxedStatic ? GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField, fieldKind) + : FieldSeqStore::NotAField(); GenTree* op1; switch (pFieldInfo->fieldAccessor) @@ -8370,7 +8373,7 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT if (isBoxedStatic) { - FieldSeqNode* outerFldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField); + FieldSeqNode* outerFldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField, fieldKind); op1->ChangeType(TYP_REF); // points at boxed object op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1, gtNewIconNode(TARGET_POINTER_SIZE, outerFldSeq)); @@ -8383,20 +8386,19 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT else { op1 = gtNewOperNode(GT_IND, lclTyp, op1); - op1->gtFlags |= GTF_GLOB_REF | GTF_IND_NONFAULTING; + op1->gtFlags |= (GTF_GLOB_REF | GTF_IND_NONFAULTING); } } return op1; } - break; } } if (isBoxedStatic) { - FieldSeqNode* outerFldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField); + FieldSeqNode* outerFldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField, fieldKind); op1 = gtNewOperNode(GT_IND, TYP_REF, op1); op1->gtFlags |= (GTF_IND_INVARIANT | GTF_IND_NONFAULTING | GTF_IND_NONNULL); diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index 999a904c3b338..b80a302ca010e 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -1008,7 +1008,8 @@ class LocalAddressVisitor final : public GenTreeVisitor // TODO-ADDR: ObjectAllocator produces FIELD nodes with FirstElemPseudoField as field // handle so we cannot use FieldSeqNode::GetFieldHandle() because it asserts on such // handles. ObjectAllocator should be changed to create LCL_FLD nodes directly. - assert(!indir->OperIs(GT_FIELD) || (indir->AsField()->gtFldHnd == fieldSeq->GetTail()->m_fieldHnd)); + assert(!indir->OperIs(GT_FIELD) || + (indir->AsField()->gtFldHnd == fieldSeq->GetTail()->GetFieldHandleValue())); } else { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 2d61a13ba4cbf..1ab54a050ab17 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5882,11 +5882,27 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) { assert(tree->gtOper == GT_FIELD); - CORINFO_FIELD_HANDLE symHnd = tree->AsField()->gtFldHnd; - unsigned fldOffset = tree->AsField()->gtFldOffset; - GenTree* objRef = tree->AsField()->GetFldObj(); - bool fieldMayOverlap = false; - bool objIsLocal = false; + CORINFO_FIELD_HANDLE symHnd = tree->AsField()->gtFldHnd; + unsigned fldOffset = tree->AsField()->gtFldOffset; + GenTree* objRef = tree->AsField()->GetFldObj(); + bool objIsLocal = false; + + FieldSeqNode* fieldSeq = FieldSeqStore::NotAField(); + if (!tree->AsField()->gtFldMayOverlap) + { + if (objRef != nullptr) + { + fieldSeq = GetFieldSeqStore()->CreateSingleton(symHnd, FieldSeqNode::FieldKind::Instance); + } + else + { + // Only simple statics get importred as GT_FIELDs. + fieldSeq = GetFieldSeqStore()->CreateSingleton(symHnd, FieldSeqNode::FieldKind::SimpleStatic); + } + } + + // Reset the flag because we may reuse the node. + tree->AsField()->gtFldMayOverlap = false; if (fgGlobalMorph && (objRef != nullptr) && (objRef->gtOper == GT_ADDR)) { @@ -5899,13 +5915,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) noway_assert(((objRef != nullptr) && (objRef->IsLocalAddrExpr() != nullptr)) || ((tree->gtFlags & GTF_GLOB_REF) != 0)); - if (tree->AsField()->gtFldMayOverlap) - { - fieldMayOverlap = true; - // Reset the flag because we may reuse the node. - tree->AsField()->gtFldMayOverlap = false; - } - #ifdef FEATURE_SIMD // if this field belongs to simd struct, translate it to simd intrinsic. if (mac == nullptr) @@ -6161,10 +6170,8 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) if (fldOffset != 0) { // Generate the "addr" node. - /* Add the member offset to the object's address */ - FieldSeqNode* fieldSeq = - fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd); - addr = gtNewOperNode(GT_ADD, (var_types)(objRefType == TYP_I_IMPL ? TYP_I_IMPL : TYP_BYREF), addr, + // Add the member offset to the object's address. + addr = gtNewOperNode(GT_ADD, (objRefType == TYP_I_IMPL) ? TYP_I_IMPL : TYP_BYREF, addr, gtNewIconHandleNode(fldOffset, GTF_ICON_FIELD_OFF, fieldSeq)); } @@ -6278,8 +6285,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) if (fldOffset != 0) { - FieldSeqNode* fieldSeq = - fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd); GenTree* fldOffsetNode = new (this, GT_CNS_INT) GenTreeIntCon(TYP_INT, fldOffset, fieldSeq); /* Add the TLS static field offset to the address */ @@ -6296,8 +6301,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) } else { - assert(!fieldMayOverlap); - // Normal static field reference // // If we can we access the static's address directly @@ -6313,9 +6316,11 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) // For boxed statics, this direct address will be for the box. We have already added // the indirection for the field itself and attached the sequence, in importation. - bool isBoxedStatic = gtIsStaticFieldPtrToBoxedStruct(tree->TypeGet(), symHnd); - FieldSeqNode* fldSeq = - !isBoxedStatic ? GetFieldSeqStore()->CreateSingleton(symHnd) : FieldSeqStore::NotAField(); + bool isBoxedStatic = gtIsStaticFieldPtrToBoxedStruct(tree->TypeGet(), symHnd); + if (isBoxedStatic) + { + fieldSeq = FieldSeqStore::NotAField(); + } // TODO-CQ: enable this optimization for 32 bit targets. bool isStaticReadOnlyInited = false; @@ -6356,7 +6361,7 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) { handleKind = GTF_ICON_STATIC_HDL; } - GenTree* addr = gtNewIconHandleNode((size_t)fldAddr, handleKind, fldSeq); + GenTree* addr = gtNewIconHandleNode((size_t)fldAddr, handleKind, fieldSeq); // Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS, if we need to. if (((tree->gtFlags & GTF_FLD_INITCLASS) != 0) && !isStaticReadOnlyInited) @@ -6392,7 +6397,7 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) static_assert_no_msg(GTF_FLD_INITCLASS == GTF_CLS_VAR_INITCLASS); tree->SetOper(GT_CLS_VAR); tree->AsClsVar()->gtClsVarHnd = symHnd; - tree->AsClsVar()->gtFieldSeq = fldSeq; + tree->AsClsVar()->gtFieldSeq = fieldSeq; } return tree; @@ -6419,8 +6424,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) assert(addr->TypeGet() == TYP_BYREF || addr->TypeGet() == TYP_I_IMPL || addr->TypeGet() == TYP_REF); // Since we don't make a constant zero to attach the field sequence to, associate it with the "addr" node. - FieldSeqNode* fieldSeq = - fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd); fgAddFieldSeqForZeroOffset(addr, fieldSeq); } @@ -13831,7 +13834,7 @@ GenTree* Compiler::fgOptimizeMultiply(GenTreeOp* mul) bool op2IsConstIndex = op2->OperGet() == GT_CNS_INT && op2->AsIntCon()->gtFieldSeq != nullptr && op2->AsIntCon()->gtFieldSeq->IsConstantIndexFieldSeq(); - assert(!op2IsConstIndex || op2->AsIntCon()->gtFieldSeq->m_next == nullptr); + assert(!op2IsConstIndex || op2->AsIntCon()->gtFieldSeq->GetNext() == nullptr); if (mult == 0) { @@ -13877,7 +13880,7 @@ GenTree* Compiler::fgOptimizeMultiply(GenTreeOp* mul) if (op2->OperGet() == GT_CNS_INT && op2->AsIntCon()->gtFieldSeq != nullptr && op2->AsIntCon()->gtFieldSeq->IsConstantIndexFieldSeq()) { - assert(op2->AsIntCon()->gtFieldSeq->m_next == nullptr); + assert(op2->AsIntCon()->gtFieldSeq->GetNext() == nullptr); GenTree* otherOp = op1; if (otherOp->OperGet() == GT_NEG) { @@ -14492,7 +14495,7 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree) if (cns->gtOper == GT_CNS_INT && cns->AsIntCon()->gtFieldSeq != nullptr && cns->AsIntCon()->gtFieldSeq->IsConstantIndexFieldSeq()) { - assert(cns->AsIntCon()->gtFieldSeq->m_next == nullptr); + assert(cns->AsIntCon()->gtFieldSeq->GetNext() == nullptr); op2->AsIntCon()->gtFieldSeq = cns->AsIntCon()->gtFieldSeq; } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 1005c50dfd415..4cefd670adbee 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -3955,7 +3955,7 @@ ValueNum ValueNumStore::VNApplySelectors(ValueNumKind vnk, FieldSeqNode* fieldSeq, size_t* wbFinalStructSize) { - for (FieldSeqNode* field = fieldSeq; field != nullptr; field = field->m_next) + for (FieldSeqNode* field = fieldSeq; field != nullptr; field = field->GetNext()) { assert(field != FieldSeqStore::NotAField()); assert(!field->IsPseudoField()); @@ -4119,7 +4119,7 @@ ValueNum ValueNumStore::VNApplySelectorsAssign( assert(fieldSeq != FieldSeqStore::NotAField()); assert(!fieldSeq->IsPseudoField()); - if (fieldSeq->m_next == nullptr) + if (fieldSeq->GetNext() == nullptr) { JITDUMP(" VNApplySelectorsAssign:\n"); } @@ -4128,10 +4128,10 @@ ValueNum ValueNumStore::VNApplySelectorsAssign( ValueNum fldHndVN = VNForFieldSelector(fieldSeq->GetFieldHandle(), &fieldType); ValueNum valueAfter; - if (fieldSeq->m_next != nullptr) + if (fieldSeq->GetNext() != nullptr) { ValueNum fseqMap = VNForMapSelect(vnk, fieldType, map, fldHndVN); - valueAfter = VNApplySelectorsAssign(vnk, fseqMap, fieldSeq->m_next, value, dstIndType); + valueAfter = VNApplySelectorsAssign(vnk, fseqMap, fieldSeq->GetNext(), value, dstIndType); } else { @@ -4182,7 +4182,7 @@ bool ValueNumStore::IsVNNotAField(ValueNum vn) // // Return Value: // "VNForNull" if the sequence is empty ("nullptr"). -// "IsVNNotAField" VN if is a "NotAField" +// "IsVNNotAField" VN if it is a "NotAField". // "GTF_FIELD_SEQ_PTR" handle VN otherwise. // ValueNum ValueNumStore::VNForFieldSeq(FieldSeqNode* fieldSeq) @@ -8006,7 +8006,7 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) ValueNum newFirstFieldValueVN = ValueNumStore::NoVN; // Optimization: avoid traversting the maps for the value of the first field if // we do not need it, which is the case if the rest of the field sequence is empty. - if (fldSeq->m_next == nullptr) + if (fldSeq->GetNext() == nullptr) { newFirstFieldValueVN = vnStore->VNApplySelectorsAssignTypeCoerce(storeVal, indType); } @@ -8018,8 +8018,9 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) firstFieldValueSelectorVN); // Construct the maps updating the struct fields in the sequence. - newFirstFieldValueVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, firstFieldValueVN, - fldSeq->m_next, storeVal, indType); + newFirstFieldValueVN = + vnStore->VNApplySelectorsAssign(VNK_Liberal, firstFieldValueVN, fldSeq->GetNext(), + storeVal, indType); } // Finally, construct the new field map... @@ -8942,7 +8943,7 @@ void Compiler::fgValueNumberTree(GenTree* tree) // Finally, account for the rest of the fields in the sequence. valueVN = - vnStore->VNApplySelectors(VNK_Liberal, firstFieldValueVN, fldSeq->m_next, &structSize); + vnStore->VNApplySelectors(VNK_Liberal, firstFieldValueVN, fldSeq->GetNext(), &structSize); } else { @@ -11080,7 +11081,7 @@ void Compiler::fgDebugCheckValueNumberedTree(GenTree* tree) break; } - fldSeq = fldSeq->m_next; + fldSeq = fldSeq->GetNext(); } assert(zeroOffsetFldSeqFound); From 1e241238ebccab98234187f085c0ef379d205f94 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 13 Mar 2022 17:29:05 +0300 Subject: [PATCH 4/7] Use shared-ness info in IsFieldAddr --- src/coreclr/jit/gentree.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index b02659419b379..618ec64e02812 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16140,17 +16140,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)) { @@ -16161,7 +16162,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()) { @@ -16173,16 +16174,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; } From 27c5d4fec74c3571b7743085ca4c10f37d07108c Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 17 Mar 2022 22:36:43 +0300 Subject: [PATCH 5/7] Revert "DEBUG-ONLY: FieldInfo (2.0)" This reverts commit b8bd75ab4877d55e4408d79402a277d994fc1cbf. --- src/coreclr/jit/compiler.cpp | 11 +--- src/coreclr/jit/compiler.h | 70 +++------------------- src/coreclr/jit/ee_il_dll.hpp | 5 -- src/coreclr/jit/gentree.cpp | 2 - src/coreclr/jit/gentree.h | 42 ------------- src/coreclr/jit/importer.cpp | 14 +---- src/coreclr/jit/importer_vectorization.cpp | 3 - src/coreclr/jit/lclvars.cpp | 8 --- src/coreclr/jit/morphblock.cpp | 6 -- 9 files changed, 13 insertions(+), 148 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 4065465d95d6c..464933b88a8e7 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -390,7 +390,7 @@ var_types Compiler::getJitGCType(BYTE gcType) // true if the given struct type contains only one pointer-sized integer value type, // false otherwise. // -bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) +bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) const { assert(info.compCompHnd->isValueClass(clsHnd)); if (info.compCompHnd->getClassSize(clsHnd) != TARGET_POINTER_SIZE) @@ -409,8 +409,6 @@ bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) CORINFO_FIELD_HANDLE fldHnd = info.compCompHnd->getFieldInClass(clsHnd, 0); CorInfoType fieldType = info.compCompHnd->getFieldType(fldHnd, pClsHnd); - INDEBUG(RecordStructFieldInfo(fldHnd)); - var_types vt = JITtype2varType(fieldType); if (fieldType == CORINFO_TYPE_VALUECLASS) @@ -1916,11 +1914,8 @@ void Compiler::compInit(ArenaAllocator* pAlloc, m_nodeTestData = nullptr; m_loopHoistCSEClass = FIRST_LOOP_HOIST_CSE_CLASS; #endif - m_switchDescMap = nullptr; - m_blockToEHPreds = nullptr; -#ifdef DEBUG - m_fieldInfoMap = nullptr; -#endif // DEBUG + m_switchDescMap = nullptr; + m_blockToEHPreds = nullptr; m_fieldSeqStore = nullptr; m_zeroOffsetFieldMap = nullptr; m_arrayInfoMap = nullptr; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 0ebf9e0d8e586..dc8bb8f80cfa4 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2648,7 +2648,7 @@ class Compiler #endif // FEATURE_MULTIREG_RET #ifdef TARGET_X86 - bool isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd); + bool isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) const; #endif // TARGET_X86 //------------------------------------------------------------------------- @@ -11135,64 +11135,20 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX void JitTestCheckVN(); // Value numbering tests. #endif // DEBUG -#ifdef DEBUG - FieldInfoMap* m_fieldInfoMap; - - FieldInfoMap* GetFieldInfoMap() - { - Compiler* rootCompiler = impInlineRoot(); - if (rootCompiler->m_fieldInfoMap == nullptr) - { - CompAllocator allocator = rootCompiler->getAllocator(CMK_DebugOnly); - rootCompiler->m_fieldInfoMap = new (allocator) FieldInfoMap(allocator); - } - - return rootCompiler->m_fieldInfoMap; - } - - void RecordFieldInfo(CORINFO_FIELD_HANDLE fieldHnd, CORINFO_FIELD_INFO* eeFieldInfo) - { - FieldInfoMap* map = GetFieldInfoMap(); - - if (!map->Lookup(fieldHnd)) - { - map->Set(fieldHnd, new (map->GetAllocator()) FieldInfo(eeFieldInfo)); - } - } - - void RecordStructFieldInfo(CORINFO_FIELD_HANDLE fieldHnd) - { - FieldInfoMap* map = GetFieldInfoMap(); - - if (!map->Lookup(fieldHnd)) - { - map->Set(fieldHnd, new (map->GetAllocator()) FieldInfo()); - } - } - - const FieldInfo* GetFieldInfo(CORINFO_FIELD_HANDLE fieldHnd) - { - const FieldInfo* fieldInfo = nullptr; - bool fieldInfoFound = GetFieldInfoMap()->Lookup(fieldHnd, &fieldInfo); - assert(fieldInfoFound); - - return fieldInfo; - } -#endif // DEBUG - // The "FieldSeqStore", for canonicalizing field sequences. See the definition of FieldSeqStore for // operations. FieldSeqStore* m_fieldSeqStore; FieldSeqStore* GetFieldSeqStore() { - Compiler* rootCompiler = impInlineRoot(); - if (rootCompiler->m_fieldSeqStore == nullptr) + Compiler* compRoot = impInlineRoot(); + if (compRoot->m_fieldSeqStore == nullptr) { - CompAllocator allocator = getAllocator(CMK_FieldSeqStore); - rootCompiler->m_fieldSeqStore = new (allocator) FieldSeqStore(allocator); + // Create a CompAllocator that labels sub-structure with CMK_FieldSeqStore, and use that for allocation. + CompAllocator ialloc(getAllocator(CMK_FieldSeqStore)); + compRoot->m_fieldSeqStore = new (ialloc) FieldSeqStore(ialloc); } - return rootCompiler->m_fieldSeqStore; + return compRoot->m_fieldSeqStore; } typedef JitHashTable, FieldSeqNode*> NodeToFieldSeqMap; @@ -11306,11 +11262,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX { m_refAnyClass = info.compCompHnd->getBuiltinClass(CLASSID_TYPED_BYREF); } - - CORINFO_FIELD_HANDLE dataFieldHnd = info.compCompHnd->getFieldInClass(m_refAnyClass, 0); - INDEBUG(RecordStructFieldInfo(dataFieldHnd)); - - return dataFieldHnd; + return info.compCompHnd->getFieldInClass(m_refAnyClass, 0); } CORINFO_FIELD_HANDLE GetRefanyTypeField() { @@ -11318,11 +11270,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX { m_refAnyClass = info.compCompHnd->getBuiltinClass(CLASSID_TYPED_BYREF); } - - CORINFO_FIELD_HANDLE typeFieldHnd = info.compCompHnd->getFieldInClass(m_refAnyClass, 1); - INDEBUG(RecordStructFieldInfo(typeFieldHnd)); - - return typeFieldHnd; + return info.compCompHnd->getFieldInClass(m_refAnyClass, 1); } #if VARSET_COUNTOPS diff --git a/src/coreclr/jit/ee_il_dll.hpp b/src/coreclr/jit/ee_il_dll.hpp index 81433dd47d67e..337ca8f147977 100644 --- a/src/coreclr/jit/ee_il_dll.hpp +++ b/src/coreclr/jit/ee_il_dll.hpp @@ -42,7 +42,6 @@ void Compiler::eeGetFieldInfo(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_FIELD_INFO* pResult) { info.compCompHnd->getFieldInfo(pResolvedToken, info.compMethodHnd, accessFlags, pResult); - INDEBUG(RecordFieldInfo(pResolvedToken->hField, pResult)); } /***************************************************************************** @@ -65,11 +64,7 @@ bool Compiler::eeIsIntrinsic(CORINFO_METHOD_HANDLE ftn) FORCEINLINE bool Compiler::eeIsFieldStatic(CORINFO_FIELD_HANDLE fldHnd) { -#ifdef DEBUG - return GetFieldInfo(fldHnd)->IsStatic(); -#else return info.compCompHnd->isFieldStatic(fldHnd); -#endif } FORCEINLINE diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 618ec64e02812..0e0f1bbffd035 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -12737,8 +12737,6 @@ GenTree* Compiler::gtFoldBoxNullable(GenTree* tree) CORINFO_CLASS_HANDLE nullableHnd = gtGetStructHandle(arg->AsOp()->gtOp1); CORINFO_FIELD_HANDLE fieldHnd = info.compCompHnd->getFieldInClass(nullableHnd, 0); - INDEBUG(RecordStructFieldInfo(fieldHnd)); - // Replace the box with an access of the nullable 'hasValue' field. JITDUMP("\nSuccess: replacing BOX_NULLABLE(&x) [%06u] with x.hasValue\n", dspTreeID(op)); GenTree* newOp = gtNewFieldRef(TYP_BOOL, fieldHnd, arg, 0); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index d73e7709d5a88..fb4003ed1a4d7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -311,48 +311,6 @@ class FieldSeqNode } }; -#ifdef DEBUG -// This class carries some about fields information neeeded for asserts from imporation to VN. -// Essentially, this is CORINFO_FIELD_INFO but Jit-specific and smaller. -class FieldInfo -{ - const unsigned m_flags; - const CORINFO_FIELD_ACCESSOR m_accessor; - -public: - FieldInfo(CORINFO_FIELD_INFO* eeFieldInfo) - : m_flags(eeFieldInfo->fieldFlags), m_accessor(eeFieldInfo->fieldAccessor) - { - } - - FieldInfo() : m_flags(0), m_accessor(CORINFO_FIELD_INSTANCE) - { - } - - bool IsStatic() const - { - return (m_flags & CORINFO_FLG_FIELD_STATIC) != 0; - } - - bool IsBoxedStatic() const - { - return (m_flags & CORINFO_FLG_FIELD_STATIC_IN_HEAP) != 0; - } - - bool IsSharedStatic() const - { - return m_accessor == CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER; - } - - bool IsReadOnly() const - { - return (m_flags & CORINFO_FLG_FIELD_FINAL) != 0; - } -}; - -typedef JitHashTable, const FieldInfo*> FieldInfoMap; -#endif // DEBUG - // This class canonicalizes field sequences. class FieldSeqStore { diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 87d08dc53440f..b0d7a3d67343e 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3712,9 +3712,6 @@ GenTree* Compiler::impCreateSpanIntrinsic(CORINFO_SIG_INFO* sig) CORINFO_FIELD_HANDLE pointerFieldHnd = info.compCompHnd->getFieldInClass(spanHnd, 0); CORINFO_FIELD_HANDLE lengthFieldHnd = info.compCompHnd->getFieldInClass(spanHnd, 1); - INDEBUG(RecordStructFieldInfo(pointerFieldHnd)); - INDEBUG(RecordStructFieldInfo(lengthFieldHnd)); - GenTreeLclFld* pointerField = gtNewLclFldNode(spanTempNum, TYP_BYREF, 0); pointerField->SetFieldSeq(GetFieldSeqStore()->CreateSingleton(pointerFieldHnd)); GenTree* pointerFieldAsg = gtNewAssignNode(pointerField, pointerValue); @@ -3997,8 +3994,6 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, assert(byReferenceStruct != nullptr); impPushOnStack(byReferenceStruct, typeInfo(TI_STRUCT, clsHnd)); retNode = assign; - - INDEBUG(RecordStructFieldInfo(fldHnd)); break; } @@ -4009,8 +4004,6 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, CORINFO_FIELD_HANDLE fldHnd = info.compCompHnd->getFieldInClass(clsHnd, 0); GenTree* field = gtNewFieldRef(TYP_BYREF, fldHnd, op1, 0); retNode = field; - - INDEBUG(RecordStructFieldInfo(fldHnd)); break; } @@ -4135,8 +4128,6 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, GenTree* length = gtNewFieldRef(TYP_INT, lengthHnd, ptrToSpan, lengthOffset); GenTree* boundsCheck = new (this, GT_BOUNDS_CHECK) GenTreeBoundsChk(index, length, SCK_RNGCHK_FAIL); - INDEBUG(RecordStructFieldInfo(lengthHnd)); - // Element access index = indexClone; @@ -4162,8 +4153,6 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, GenTree* data = gtNewFieldRef(TYP_BYREF, ptrHnd, ptrToSpanClone, ptrOffset); GenTree* result = gtNewOperNode(GT_ADD, TYP_BYREF, data, index); - INDEBUG(RecordStructFieldInfo(ptrHnd)); - // Prepare result var_types resultType = JITtype2varType(sig->retType); assert(resultType == result->TypeGet()); @@ -7121,8 +7110,6 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, const CORINFO_FIELD_HANDLE hasValueFldHnd = info.compCompHnd->getFieldInClass(nullableCls, 0); - INDEBUG(RecordStructFieldInfo(hasValueFldHnd)); - assert(info.compCompHnd->getFieldOffset(hasValueFldHnd) == 0); assert(!strcmp(info.compCompHnd->getFieldName(hasValueFldHnd, nullptr), "hasValue")); @@ -8247,6 +8234,7 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT : FieldSeqStore::NotAField(); GenTree* op1; + switch (pFieldInfo->fieldAccessor) { case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER: diff --git a/src/coreclr/jit/importer_vectorization.cpp b/src/coreclr/jit/importer_vectorization.cpp index 192fa4e68a113..00a5389a8939e 100644 --- a/src/coreclr/jit/importer_vectorization.cpp +++ b/src/coreclr/jit/importer_vectorization.cpp @@ -698,9 +698,6 @@ GenTree* Compiler::impSpanEqualsOrStartsWith(bool startsWith, CORINFO_SIG_INFO* CORINFO_FIELD_HANDLE lengthHnd = info.compCompHnd->getFieldInClass(spanCls, 1); const unsigned lengthOffset = info.compCompHnd->getFieldOffset(lengthHnd); - INDEBUG(RecordStructFieldInfo(pointerHnd)); - INDEBUG(RecordStructFieldInfo(lengthHnd)); - // Create a placeholder for Span object - we're not going to Append it to statements // in advance to avoid redundant spills in case if we fail to vectorize unsigned spanObjRef = lvaGrabTemp(true DEBUGARG("spanObj tmp")); diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 28468efd8803a..df32fcf45dc85 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -1730,8 +1730,6 @@ bool Compiler::StructPromotionHelper::CanPromoteStructType(CORINFO_CLASS_HANDLE structPromotionInfo.fields[ordinal] = GetFieldInfo(fieldHnd, ordinal); const lvaStructFieldInfo& fieldInfo = structPromotionInfo.fields[ordinal]; - INDEBUG(compiler->RecordStructFieldInfo(fieldHnd)); - noway_assert(fieldInfo.fldOffset < structSize); if (fieldInfo.fldSize == 0) @@ -2194,9 +2192,6 @@ bool Compiler::StructPromotionHelper::TryPromoteStructField(lvaStructFieldInfo& // the struct field. CORINFO_FIELD_HANDLE innerFieldHndl = compHandle->getFieldInClass(fieldInfo.fldTypeHnd, 0); unsigned innerFieldOffset = compHandle->getFieldOffset(innerFieldHndl); - - INDEBUG(compiler->RecordStructFieldInfo(innerFieldHndl)); - if (innerFieldOffset != 0) { return false; @@ -2838,9 +2833,6 @@ void Compiler::makeExtraStructQueries(CORINFO_CLASS_HANDLE structHandle, int lev CORINFO_CLASS_HANDLE fieldClassHandle = NO_CLASS_HANDLE; CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHandle, &fieldClassHandle); var_types fieldVarType = JITtype2varType(fieldCorType); - - INDEBUG(RecordStructFieldInfo(fieldHandle)); - if (fieldClassHandle != NO_CLASS_HANDLE) { if (varTypeIsStruct(fieldVarType)) diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 000137c8e8bbe..b8c12020b306f 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -1237,9 +1237,6 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() CORINFO_CLASS_HANDLE classHnd = srcVarDsc->GetStructHnd(); CORINFO_FIELD_HANDLE fieldHnd = m_comp->info.compCompHnd->getFieldInClass(classHnd, srcFieldVarDsc->lvFldOrdinal); - - INDEBUG(m_comp->RecordStructFieldInfo(fieldHnd)); - FieldSeqNode* curFieldSeq = m_comp->GetFieldSeqStore()->CreateSingleton(fieldHnd); unsigned srcFieldOffset = m_comp->lvaGetDesc(srcFieldLclNum)->lvFldOffset; @@ -1345,9 +1342,6 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() CORINFO_FIELD_HANDLE fieldHnd = m_comp->info.compCompHnd->getFieldInClass(classHnd, m_comp->lvaGetDesc(dstFieldLclNum)->lvFldOrdinal); - - INDEBUG(m_comp->RecordStructFieldInfo(fieldHnd)); - FieldSeqNode* curFieldSeq = m_comp->GetFieldSeqStore()->CreateSingleton(fieldHnd); var_types destType = m_comp->lvaGetDesc(dstFieldLclNum)->lvType; From ba5f6649713f0bb5459f0aa2449d66a5ec720202 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 29 Mar 2022 12:19:04 +0300 Subject: [PATCH 6/7] Revert unintentional assert change --- src/coreclr/jit/gentree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0e0f1bbffd035..32d52b5502656 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17211,7 +17211,7 @@ void GenTree::ParseArrayAddress( *pFldSeq = fldSeqIter; } CORINFO_CLASS_HANDLE fldCls = nullptr; - noway_assert(fldSeqIter->GetNext() != nullptr); + noway_assert(fldSeqIter->GetFieldHandle() != NO_CLASS_HANDLE); CorInfoType cit = comp->info.compCompHnd->getFieldType(fldSeqIter->GetFieldHandle(), &fldCls); fieldOffsets += comp->compGetTypeSize(cit, fldCls); } From 66c1d3b52c3323e8b4bc11802b396f36843cc489 Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Tue, 29 Mar 2022 14:15:35 +0300 Subject: [PATCH 7/7] Fix build --- src/coreclr/jit/gentree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 32d52b5502656..aa96e7d3d5293 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17211,7 +17211,7 @@ void GenTree::ParseArrayAddress( *pFldSeq = fldSeqIter; } CORINFO_CLASS_HANDLE fldCls = nullptr; - noway_assert(fldSeqIter->GetFieldHandle() != NO_CLASS_HANDLE); + noway_assert(fldSeqIter->GetFieldHandle() != NO_FIELD_HANDLE); CorInfoType cit = comp->info.compCompHnd->getFieldType(fldSeqIter->GetFieldHandle(), &fldCls); fieldOffsets += comp->compGetTypeSize(cit, fldCls); }