Proper VNs for zeroed structs (#61285)
authorSingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Tue, 9 Nov 2021 19:12:49 +0000 (22:12 +0300)
committerGitHub <noreply@github.com>
Tue, 9 Nov 2021 19:12:49 +0000 (20:12 +0100)
* 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.

* 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/compiler.h
src/coreclr/jit/ee_il_dll.hpp
src/coreclr/jit/valuenum.cpp
src/coreclr/jit/valuenum.h
src/coreclr/jit/valuenumfuncs.h

index 188c583..11bca2a 100644 (file)
@@ -7986,7 +7986,7 @@ public:
     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)
 
index 5f98a70..abedfa4 100644 (file)
@@ -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
index 41015c0..385cd20 100644 (file)
@@ -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,15 +1808,18 @@ 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:
         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.
@@ -1829,6 +1828,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)
@@ -1856,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};
 
 //----------------------------------------------------------------------------------------
@@ -2317,14 +2337,9 @@ TailCall:
             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 +2491,22 @@ TailCall:
                     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<ssize_t>(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
@@ -5827,11 +5858,6 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr)
                 {
                     printf("void");
                 }
-                else
-                {
-                    assert(vn == VNForZeroMap());
-                    printf("zeroMap");
-                }
                 break;
             case TYP_BYREF:
                 printf("byrefVal");
@@ -5903,6 +5929,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));
@@ -6124,6 +6153,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<ssize_t>(zeroObj->m_args[0]))));
+}
 #endif // DEBUG
 
 // Static fields, methods.
@@ -6287,10 +6322,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
@@ -6715,7 +6749,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
                     {
@@ -7975,39 +8010,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");
                 }
@@ -9525,9 +9559,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
@@ -9642,9 +9674,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
index 8811ea2..40ab7b0 100644 (file)
@@ -420,13 +420,6 @@ public:
         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,10 +456,18 @@ public:
     // 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);
 
+#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);
@@ -1030,6 +1031,9 @@ public:
     // 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.
@@ -1458,7 +1462,6 @@ private:
     enum SpecialRefConsts
     {
         SRC_Null,
-        SRC_ZeroMap,
         SRC_ReadOnlyHeap,
         SRC_Void,
         SRC_EmptyExcSet,
index 7665770..2d7a70d 100644 (file)
@@ -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.