From 132f31fc270dca6e312ecafb2f9c3352518d227d Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 5 Nov 2021 18:53:48 +0300 Subject: [PATCH 1/2] Number zero-initialized struct properly Previously, zero-initialized struct locals were given a special $VNForZeroMap, which had the nice property that VNForMapSelect($VNForZeroMap, Field) would always return zero of the appropriate type. However, the fact that this VN is not unique meant that it was dangerous to propagate it, e. g. through copies, which was hidden by the fact that ApplySelectorsTypeCheck's logic is very conservative. This change introduces a new VNFunc to represent zeroed objects - VNF_ZeroObj, that takes a struct handle as its argument and is thus free of the aforementioned problem and can be propagated freely and not treated specially. It also, of course, retains the ZeroMap's selection property. There are almost no diffs for this change because of some other deficiencies in assertion and copy propagation, they are being/will be fixed separately. --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/ee_il_dll.hpp | 4 +- src/coreclr/jit/valuenum.cpp | 106 +++++++++++++++++++------------- src/coreclr/jit/valuenum.h | 14 ++--- src/coreclr/jit/valuenumfuncs.h | 2 +- 5 files changed, 72 insertions(+), 56 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index befb4a6f60b7b7..3928242b0fa26d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7986,7 +7986,7 @@ class Compiler bool eeIsJitIntrinsic(CORINFO_METHOD_HANDLE ftn); bool eeIsFieldStatic(CORINFO_FIELD_HANDLE fldHnd); - var_types eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd); + var_types eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd, CORINFO_CLASS_HANDLE* pStructHnd = nullptr); #if defined(DEBUG) || defined(FEATURE_JIT_METHOD_PERF) || defined(FEATURE_SIMD) || defined(TRACK_LSRA_STATS) diff --git a/src/coreclr/jit/ee_il_dll.hpp b/src/coreclr/jit/ee_il_dll.hpp index 5f98a701468521..abedfa4ddf0778 100644 --- a/src/coreclr/jit/ee_il_dll.hpp +++ b/src/coreclr/jit/ee_il_dll.hpp @@ -68,9 +68,9 @@ bool Compiler::eeIsFieldStatic(CORINFO_FIELD_HANDLE fldHnd) } FORCEINLINE -var_types Compiler::eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd) +var_types Compiler::eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd, CORINFO_CLASS_HANDLE* pStructHnd) { - return JITtype2varType(info.compCompHnd->getFieldType(fldHnd)); + return JITtype2varType(info.compCompHnd->getFieldType(fldHnd, pStructHnd)); } FORCEINLINE diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 8d255447da77ef..544fa7479485e0 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -464,11 +464,9 @@ ValueNumStore::ValueNumStore(Compiler* comp, CompAllocator alloc) { m_VNsForSmallIntConsts[i] = NoVN; } - // We will reserve chunk 0 to hold some special constants, like the constant NULL, the "exception" value, and the - // "zero map." + // We will reserve chunk 0 to hold some special constants. Chunk* specialConstChunk = new (m_alloc) Chunk(m_alloc, &m_nextChunkBase, TYP_REF, CEA_Const); - specialConstChunk->m_numUsed += - SRC_NumSpecialRefConsts; // Implicitly allocate 0 ==> NULL, and 1 ==> Exception, 2 ==> ZeroMap. + specialConstChunk->m_numUsed += SRC_NumSpecialRefConsts; ChunkNum cn = m_chunks.Push(specialConstChunk); assert(cn == 0); @@ -1787,8 +1785,6 @@ ValueNum ValueNumStore::VNForHandle(ssize_t cnsVal, GenTreeFlags handleFlags) } } -// Returns the value number for zero of the given "typ". -// It has an unreached() for a "typ" that has no zero value, such as TYP_VOID. ValueNum ValueNumStore::VNZeroForType(var_types typ) { switch (typ) @@ -1812,8 +1808,6 @@ ValueNum ValueNumStore::VNZeroForType(var_types typ) return VNForNull(); case TYP_BYREF: return VNForByrefCon(0); - case TYP_STRUCT: - return VNForZeroMap(); // Recursion! #ifdef FEATURE_SIMD case TYP_SIMD8: @@ -1829,6 +1823,16 @@ ValueNum ValueNumStore::VNZeroForType(var_types typ) } } +ValueNum ValueNumStore::VNForZeroObj(CORINFO_CLASS_HANDLE structHnd) +{ + assert(structHnd != NO_CLASS_HANDLE); + + ValueNum structHndVN = VNForHandle(ssize_t(structHnd), GTF_ICON_CLASS_HDL); + ValueNum zeroObjVN = VNForFunc(TYP_STRUCT, VNF_ZeroObj, structHndVN); + + return zeroObjVN; +} + // Returns the value number for one of the given "typ". // It returns NoVN for a "typ" that has no one value, such as TYP_REF. ValueNum ValueNumStore::VNOneForType(var_types typ) @@ -2317,14 +2321,9 @@ ValueNum ValueNumStore::VNForMapSelectWork( return RecursiveVN; } - if (map == VNForZeroMap()) - { - return VNZeroForType(type); - } - else if (IsVNFunc(map)) + VNFuncApp funcApp; + if (GetVNFunc(map, &funcApp)) { - VNFuncApp funcApp; - GetVNFunc(map, &funcApp); if (funcApp.m_func == VNF_MapStore) { // select(store(m, i, v), i) == v @@ -2476,6 +2475,22 @@ ValueNum ValueNumStore::VNForMapSelectWork( m_fixedPointMapSels.Pop(); } } + else if (funcApp.m_func == VNF_ZeroObj) + { + // For structs, we need to extract the handle from the selector. + if (type == TYP_STRUCT) + { + // We only expect field selectors here. + assert(GetHandleFlags(index) == GTF_ICON_FIELD_HDL); + CORINFO_FIELD_HANDLE fieldHnd = CORINFO_FIELD_HANDLE(ConstantValue(index)); + CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE; + m_pComp->eeGetFieldType(fieldHnd, &structHnd); + + return VNForZeroObj(structHnd); + } + + return VNZeroForType(type); + } } // We may have run out of budget and already assigned a result @@ -5792,11 +5807,6 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) { printf("void"); } - else - { - assert(vn == VNForZeroMap()); - printf("zeroMap"); - } break; case TYP_BYREF: printf("byrefVal"); @@ -5868,6 +5878,9 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) case VNF_CastOvf: vnDumpCast(comp, vn); break; + case VNF_ZeroObj: + vnDumpZeroObj(comp, &funcApp); + break; default: printf("%s(", VNFuncName(funcApp.m_func)); @@ -6089,6 +6102,12 @@ void ValueNumStore::vnDumpCast(Compiler* comp, ValueNum castVN) } } +void ValueNumStore::vnDumpZeroObj(Compiler* comp, VNFuncApp* zeroObj) +{ + printf("ZeroObj("); + comp->vnPrint(zeroObj->m_args[0], 0); + printf(": %s)", comp->eeGetClassName(CORINFO_CLASS_HANDLE(ConstantValue(zeroObj->m_args[0])))); +} #endif // DEBUG // Static fields, methods. @@ -6252,10 +6271,9 @@ static const char* s_reservedNameArr[] = { "$VN.Recursive", // -2 RecursiveVN "$VN.No", // -1 NoVN "$VN.Null", // 0 VNForNull() - "$VN.ZeroMap", // 1 VNForZeroMap() - "$VN.ReadOnlyHeap", // 2 VNForROH() - "$VN.Void", // 3 VNForVoid() - "$VN.EmptyExcSet" // 4 VNForEmptyExcSet() + "$VN.ReadOnlyHeap", // 1 VNForROH() + "$VN.Void", // 2 VNForVoid() + "$VN.EmptyExcSet" // 3 VNForEmptyExcSet() }; // Returns the string name of "vn" when it is a reserved value number, nullptr otherwise @@ -6680,7 +6698,8 @@ void Compiler::fgValueNumber() if (isZeroed) { // By default we will zero init these LclVars - initVal = vnStore->VNZeroForType(typ); + initVal = (typ == TYP_STRUCT) ? vnStore->VNForZeroObj(varDsc->GetStructHnd()) + : vnStore->VNZeroForType(typ); } else { @@ -7942,39 +7961,38 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) // Should not have been recorded as updating the GC heap. assert(!GetMemorySsaMap(GcHeap)->Lookup(tree)); - unsigned lclNum = lclVarTree->GetLclNum(); - unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree); + // Ignore vars that we excluded from SSA (for example, because they're address-exposed). They don't have // SSA names in which to store VN's on defs. We'll yield unique VN's when we read from them. - if (lvaInSsa(lclNum) && lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM) + if (lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM) { + LclVarDsc* lclVarDsc = lvaGetDesc(lclVarTree); + // Should not have been recorded as updating ByrefExposed. assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree)); - ValueNum initBlkVN = ValueNumStore::NoVN; - GenTree* initConst = rhs; - if (isEntire && initConst->OperGet() == GT_CNS_INT) + ValueNum lclVarVN = ValueNumStore::NoVN; + if (isEntire && rhs->IsIntegralConst(0)) { - unsigned initVal = 0xFF & (unsigned)initConst->AsIntConCommon()->IconValue(); - if (initVal == 0) - { - initBlkVN = vnStore->VNZeroForType(lclVarTree->TypeGet()); - } + // Note that it is possible to see pretty much any kind of type for the local + // (not just TYP_STRUCT) here because of the ASG(BLK(ADDR(LCL_VAR/FLD)), 0) form. + lclVarVN = (lclVarDsc->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(lclVarDsc->GetStructHnd()) + : vnStore->VNZeroForType(lclVarDsc->TypeGet()); + } + else + { + // Non-zero block init is very rare so we'll use a simple, unique VN here. + lclVarVN = vnStore->VNForExpr(compCurBB, lclVarDsc->TypeGet()); } - ValueNum lclVarVN = (initBlkVN != ValueNumStore::NoVN) - ? initBlkVN - : vnStore->VNForExpr(compCurBB, var_types(lvaTable[lclNum].lvType)); - lvaTable[lclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair.SetBoth(lclVarVN); + lclVarDsc->GetPerSsaData(lclDefSsaNum)->m_vnPair.SetBoth(lclVarVN); #ifdef DEBUG if (verbose) { - printf("N%03u ", tree->gtSeqNum); + printf("Tree "); Compiler::printTreeID(tree); - printf(" "); - gtDispNodeName(tree); - printf(" V%02u/%d => ", lclNum, lclDefSsaNum); + printf(" assigned VN to local var V%02u/%d: ", lclVarTree->GetLclNum(), lclDefSsaNum); vnPrint(lclVarVN, 1); printf("\n"); } diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index afb9510090e39a..8de822c023951d 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -420,13 +420,6 @@ class ValueNumStore return ValueNum(SRC_Null); } - // The zero map is the map that returns a zero "for the appropriate type" when indexed at any index. - static ValueNum VNForZeroMap() - { - // We reserve Chunk 0 for "special" VNs. Let SRC_ZeroMap (== 1) be the zero map. - return ValueNum(SRC_ZeroMap); - } - // The ROH map is the map for the "read-only heap". We assume that this is never mutated, and always // has the same value number. static ValueNum VNForROH() @@ -463,6 +456,9 @@ class ValueNumStore // It has an unreached() for a "typ" that has no zero value, such as TYP_VOID. ValueNum VNZeroForType(var_types typ); + // Returns the value number for a zero-initialized struct. + ValueNum VNForZeroObj(CORINFO_CLASS_HANDLE structHnd); + // Returns the value number for one of the given "typ". // It returns NoVN for a "typ" that has no one value, such as TYP_REF. ValueNum VNOneForType(var_types typ); @@ -1021,6 +1017,9 @@ class ValueNumStore // Prints the cast's representation mirroring GT_CAST's dump format. void vnDumpCast(Compiler* comp, ValueNum castVN); + // Requires "zeroObj" to be a VNF_ZeroObj. Prints its representation. + void vnDumpZeroObj(Compiler* comp, VNFuncApp* zeroObj); + // Returns the string name of "vnf". static const char* VNFuncName(VNFunc vnf); // Used in the implementation of the above. @@ -1449,7 +1448,6 @@ class ValueNumStore enum SpecialRefConsts { SRC_Null, - SRC_ZeroMap, SRC_ReadOnlyHeap, SRC_Void, SRC_EmptyExcSet, diff --git a/src/coreclr/jit/valuenumfuncs.h b/src/coreclr/jit/valuenumfuncs.h index 7665770881bcd9..2d7a70dbc7480d 100644 --- a/src/coreclr/jit/valuenumfuncs.h +++ b/src/coreclr/jit/valuenumfuncs.h @@ -12,7 +12,6 @@ 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(ZeroMap, 0, false, false, false) // The "ZeroMap": indexing at any index yields "zero of the desired type". ValueNumFuncDef(PtrToLoc, 2, false, false, 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. @@ -22,6 +21,7 @@ ValueNumFuncDef(Phi, 2, false, false, false) // A phi function. Only occ ValueNumFuncDef(PhiDef, 3, false, false, false) // Args: 0: local var # (or -1 for memory), 1: SSA #, 2: VN of definition. // Wouldn't need this if I'd made memory a regular local variable... ValueNumFuncDef(PhiMemoryDef, 2, false, false, false) // Args: 0: VN for basic block pointer, 1: VN of definition +ValueNumFuncDef(ZeroObj, 1, false, false, false) // Zero-initialized struct. Args: 0: VN of the class handle. ValueNumFuncDef(InitVal, 1, false, false, false) // An input arg, or init val of a local Args: 0: a constant VN. From c8843720b05aa097ac372b7871a880b805e392a9 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sat, 6 Nov 2021 02:47:37 +0300 Subject: [PATCH 2/2] Fix VNZeroForType's handling of SIMD types Previously this codepath was reachable, but did not matter as the returned values were immediately discarded because of the conservative logic in VNApplySelectorsTypeCheck. This is still more or less the case, but let's just fix it properly. --- src/coreclr/jit/valuenum.cpp | 27 ++++++++++++++++++++------- src/coreclr/jit/valuenum.h | 5 +++++ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 544fa7479485e0..afeaf0bcccd8a4 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -1814,7 +1814,12 @@ ValueNum ValueNumStore::VNZeroForType(var_types typ) case TYP_SIMD12: case TYP_SIMD16: case TYP_SIMD32: - return VNForLongCon(0); + // We do not have the base type - a "fake" one will have to do. Note that we cannot + // use the HWIntrinsic "get_Zero" VNFunc here. This is because they only represent + // "fully zeroed" vectors, and here we may be loading one from memory, leaving upper + // bits undefined. So using "SIMD_Init" is "the next best thing", so to speak, and + // TYP_FLOAT is one of the more popular base types, so that's why we use it here. + return VNForFunc(typ, VNF_SIMD_Init, VNForFloatCon(0), VNForSimdType(genTypeSize(typ), TYP_FLOAT)); #endif // FEATURE_SIMD // These should be unreached. @@ -1860,6 +1865,17 @@ ValueNum ValueNumStore::VNOneForType(var_types typ) } } +#ifdef FEATURE_SIMD +ValueNum ValueNumStore::VNForSimdType(unsigned simdSize, var_types simdBaseType) +{ + ValueNum baseTypeVN = VNForIntCon(INT32(simdBaseType)); + ValueNum sizeVN = VNForIntCon(simdSize); + ValueNum simdTypeVN = VNForFunc(TYP_REF, VNF_SimdType, sizeVN, baseTypeVN); + + return simdTypeVN; +} +#endif // FEATURE_SIMD + class Object* ValueNumStore::s_specialRefConsts[] = {nullptr, nullptr, nullptr}; //---------------------------------------------------------------------------------------- @@ -9510,9 +9526,7 @@ void Compiler::fgValueNumberSimd(GenTree* tree) if (encodeResultType) { - ValueNum vnSize = vnStore->VNForIntCon(simdNode->GetSimdSize()); - ValueNum vnBaseType = vnStore->VNForIntCon(INT32(simdNode->GetSimdBaseType())); - ValueNum simdTypeVN = vnStore->VNForFunc(TYP_REF, VNF_SimdType, vnSize, vnBaseType); + ValueNum simdTypeVN = vnStore->VNForSimdType(simdNode->GetSimdSize(), simdNode->GetSimdBaseType()); resvnp.SetBoth(simdTypeVN); #ifdef DEBUG @@ -9627,9 +9641,8 @@ void Compiler::fgValueNumberHWIntrinsic(GenTree* tree) if (encodeResultType) { - ValueNum vnSize = vnStore->VNForIntCon(hwIntrinsicNode->GetSimdSize()); - ValueNum vnBaseType = vnStore->VNForIntCon(INT32(hwIntrinsicNode->GetSimdBaseType())); - ValueNum simdTypeVN = vnStore->VNForFunc(TYP_REF, VNF_SimdType, vnSize, vnBaseType); + ValueNum simdTypeVN = + vnStore->VNForSimdType(hwIntrinsicNode->GetSimdSize(), hwIntrinsicNode->GetSimdBaseType()); resvnp.SetBoth(simdTypeVN); #ifdef DEBUG diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 8de822c023951d..4e22f80d212508 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -463,6 +463,11 @@ class ValueNumStore // It returns NoVN for a "typ" that has no one value, such as TYP_REF. ValueNum VNOneForType(var_types typ); +#ifdef FEATURE_SIMD + // A helper function for constructing VNF_SimdType VNs. + ValueNum VNForSimdType(unsigned simdSize, var_types simdBaseType); +#endif // FEATURE_SIMD + // Create or return the existimg value number representing a singleton exception set // for the the exception value "x". ValueNum VNExcSetSingleton(ValueNum x);