Stop using CLS_VAR for "boxed statics"

src/coreclr/jit/gentree.cpp

@@ -15917,6 +15917,9 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
     // will effectively treat such cases ("possible" in unsafe code) as undefined behavior.
     if (comp->eeIsFieldStatic(fldSeq->GetFieldHandle()))
     {
+        // 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".
+
         // 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.

src/coreclr/jit/morph.cpp

@@ -6265,20 +6265,32 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac)
             FieldSeqNode* fldSeq =
                 !isBoxedStatic ? GetFieldSeqStore()->CreateSingleton(symHnd) : FieldSeqStore::NotAField();
 
-#ifdef TARGET_64BIT
+            // TODO-CQ: enable this optimization for 32 bit targets.
             bool isStaticReadOnlyInited = false;
-            bool plsSpeculative         = true;
-            if (info.compCompHnd->getStaticFieldCurrentClass(symHnd, &plsSpeculative) != NO_CLASS_HANDLE)
+#ifdef TARGET_64BIT
+            if (tree->TypeIs(TYP_REF) && !isBoxedStatic)
             {
-                isStaticReadOnlyInited = !plsSpeculative;
+                bool pIsSpeculative = true;
+                if (info.compCompHnd->getStaticFieldCurrentClass(symHnd, &pIsSpeculative) != NO_CLASS_HANDLE)
+                {
+                    isStaticReadOnlyInited = !pIsSpeculative;
+                }
             }
+#endif // TARGET_64BIT
 
-            // even if RelocTypeHint is REL32 let's still prefer IND over GT_CLS_VAR
-            // for static readonly fields of statically initialized classes - thus we can
-            // apply GTF_IND_INVARIANT flag and make it hoistable/CSE-friendly
-            if (isStaticReadOnlyInited || (IMAGE_REL_BASED_REL32 != eeGetRelocTypeHint(fldAddr)))
+            // TODO: choices made below have mostly historical reasons and
+            // should be unified to always use the IND(<address>) form.
+            CLANG_FORMAT_COMMENT_ANCHOR;
+
+#ifdef TARGET_64BIT
+            bool preferIndir =
+                isBoxedStatic || isStaticReadOnlyInited || (IMAGE_REL_BASED_REL32 != eeGetRelocTypeHint(fldAddr));
+#else  // !TARGET_64BIT
+            bool preferIndir = isBoxedStatic;
+#endif // !TARGET_64BIT
+
+            if (preferIndir)
             {
-                // The address is not directly addressible, so force it into a constant, so we handle it properly.
                 GenTreeFlags handleKind = GTF_EMPTY;
                 if (isBoxedStatic)
                 {
@@ -6321,7 +6333,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac)
                 return fgMorphSmpOp(tree);
             }
             else
-#endif // TARGET_64BIT
             {
                 // Only volatile or classinit could be set, and they map over
                 noway_assert((tree->gtFlags & ~(GTF_FLD_VOLATILE | GTF_FLD_INITCLASS | GTF_COMMON_MASK)) == 0);

src/coreclr/jit/valuenum.cpp

@@ -7802,8 +7802,24 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree)
                 GenTree*      baseAddr = nullptr;
                 FieldSeqNode* fldSeq   = nullptr;
 
+                if (argIsVNFunc && funcApp.m_func == VNF_PtrToStatic)
+                {
+                    FieldSeqNode* fldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]);
+                    assert(fldSeq != nullptr); // We should never see an empty sequence here.
+
+                    if (fldSeq != FieldSeqStore::NotAField())
+                    {
+                        ValueNum newHeapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], fldSeq,
+                                                                             rhsVNPair.GetLiberal(), lhs->TypeGet());
+                        recordGcHeapStore(tree, newHeapVN DEBUGARG("static field store"));
+                    }
+                    else
+                    {
+                        fgMutateGcHeap(tree DEBUGARG("indirect store at NotAField PtrToStatic address"));
+                    }
+                }
                 // Is the LHS an array index expression?
-                if (argIsVNFunc && funcApp.m_func == VNF_PtrToArrElem)
+                else if (argIsVNFunc && funcApp.m_func == VNF_PtrToArrElem)
                 {
                     CORINFO_CLASS_HANDLE elemTypeEq =
                         CORINFO_CLASS_HANDLE(vnStore->ConstantValue<ssize_t>(funcApp.m_args[0]));
@@ -8625,39 +8641,19 @@ void Compiler::fgValueNumberTree(GenTree* tree)
                     ValueNumPair   clsVarVNPair;
                     GenTreeClsVar* clsVar = tree->AsClsVar();
                     FieldSeqNode*  fldSeq = clsVar->gtFieldSeq;
-                    assert(fldSeq != nullptr); // We need to have one.
-                    CORINFO_FIELD_HANDLE fieldHnd = clsVar->gtClsVarHnd;
-                    assert(fieldHnd != NO_FIELD_HANDLE);
-                    ValueNum selectedStaticVar = ValueNumStore::NoVN;
-
-                    if (fldSeq == FieldSeqStore::NotAField())
-                    {
-                        // This is the box for a "boxed static" - see "fgMorphField".
-                        assert(gtIsStaticFieldPtrToBoxedStruct(clsVar->TypeGet(), fieldHnd));
-
-                        // We will create an empty field sequence for VNF_PtrToStatic here. We will assume
-                        // the actual sequence will get appended later, when processing the TARGET_POINTER_SIZE
-                        // offset that is always added to this box to get to its payload.
+                    assert((fldSeq != nullptr) && (fldSeq != FieldSeqStore::NotAField())); // We need to have one.
 
-                        ValueNum fieldHndVN = vnStore->VNForHandle(ssize_t(fieldHnd), GTF_ICON_FIELD_HDL);
-                        ValueNum fieldSeqVN = vnStore->VNForFieldSeq(nullptr);
-                        clsVarVNPair.SetBoth(vnStore->VNForFunc(TYP_REF, VNF_PtrToStatic, fieldHndVN, fieldSeqVN));
-                    }
-                    else
-                    {
-                        // This is a reference to heap memory.
-                        // We model statics as indices into GcHeap (which is a subset of ByrefExposed).
+                    // This is a reference to heap memory.
+                    // We model statics as indices into GcHeap (which is a subset of ByrefExposed).
+                    size_t   structSize = 0;
+                    ValueNum selectedStaticVar =
+                        vnStore->VNApplySelectors(VNK_Liberal, fgCurMemoryVN[GcHeap], fldSeq, &structSize);
+                    selectedStaticVar =
+                        vnStore->VNApplySelectorsTypeCheck(selectedStaticVar, tree->TypeGet(), structSize);
 
-                        size_t structSize = 0;
-                        selectedStaticVar =
-                            vnStore->VNApplySelectors(VNK_Liberal, fgCurMemoryVN[GcHeap], fldSeq, &structSize);
-                        selectedStaticVar =
-                            vnStore->VNApplySelectorsTypeCheck(selectedStaticVar, tree->TypeGet(), structSize);
-
-                        clsVarVNPair.SetLiberal(selectedStaticVar);
-                        // The conservative interpretation always gets a new, unique VN.
-                        clsVarVNPair.SetConservative(vnStore->VNForExpr(compCurBB, tree->TypeGet()));
-                    }
+                    clsVarVNPair.SetLiberal(selectedStaticVar);
+                    // The conservative interpretation always gets a new, unique VN.
+                    clsVarVNPair.SetConservative(vnStore->VNForExpr(compCurBB, tree->TypeGet()));
 
                     // The ValueNum returned must represent the full-sized IL-Stack value
                     // If we need to widen this value then we need to introduce a VNF_Cast here to represent
@@ -8834,9 +8830,22 @@ void Compiler::fgValueNumberTree(GenTree* tree)
 
                 if (!wasNewobj)
                 {
+                    // Indirections off of addresses for boxed statics represent bases for
+                    // the address of the static itself. Here we will use "nullptr" for the
+                    // field sequence and assume the actual static field will be appended to
+                    // it later, as part of numbering the method table pointer offset addition.
+                    if (addr->IsCnsIntOrI() && addr->IsIconHandle(GTF_ICON_STATIC_BOX_PTR))
+                    {
+                        assert(addrNvnp.BothEqual() && (addrXvnp == vnStore->VNPForEmptyExcSet()));
+                        ValueNum boxAddrVN  = addrNvnp.GetLiberal();
+                        ValueNum fieldSeqVN = vnStore->VNForFieldSeq(nullptr);
+                        ValueNum staticAddrVN =
+                            vnStore->VNForFunc(tree->TypeGet(), VNF_PtrToStatic, boxAddrVN, fieldSeqVN);
+                        tree->gtVNPair = ValueNumPair(staticAddrVN, staticAddrVN);
+                    }
                     // Is this invariant indirect expected to always return a non-null value?
                     // TODO-VNTypes: non-null indirects should only be used for TYP_REFs.
-                    if ((tree->gtFlags & GTF_IND_NONNULL) != 0)
+                    else if ((tree->gtFlags & GTF_IND_NONNULL) != 0)
                     {
                         assert(tree->gtFlags & GTF_IND_NONFAULTING);
                         tree->gtVNPair = vnStore->VNPairForFunc(tree->TypeGet(), VNF_NonNullIndirect, addrNvnp);
@@ -10614,8 +10623,7 @@ void Compiler::fgValueNumberAddExceptionSetForIndirection(GenTree* tree, GenTree
 
     // The normal VN for base address is used to create the NullPtrExc
     ValueNumPair vnpBaseNorm = vnStore->VNPNormalPair(baseVNP);
-
-    ValueNumPair excChkSet = vnStore->VNPForEmptyExcSet();
+    ValueNumPair excChkSet   = vnStore->VNPForEmptyExcSet();
 
     if (!vnStore->IsKnownNonNull(vnpBaseNorm.GetLiberal()))
     {

src/coreclr/jit/valuenumfuncs.h

@@ -16,7 +16,7 @@ ValueNumFuncDef(NotAField, 0, false, false, false)  // Value number function for
 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).
-                                                            // Args: 0: (VN of) the field handle, 1: the field sequence, of which the first element is the static itself.
+                                                            // Args: 0: (VN of) the box's address if the static is "boxed", 1: the field sequence, of which the first element is the static itself.
 
 ValueNumFuncDef(Phi, 2, false, false, false)        // A phi function.  Only occurs as arg of PhiDef or PhiMemoryDef.  Arguments are SSA numbers of var being defined.
 ValueNumFuncDef(PhiDef, 3, false, false, false)     // Args: 0: local var # (or -1 for memory), 1: SSA #, 2: VN of definition.

src/coreclr/jit/valuenumtype.h

@@ -96,6 +96,16 @@ struct ValueNumPair
         m_conservative = vn;
     }
 
+    bool operator==(const ValueNumPair& other) const
+    {
+        return (m_liberal == other.m_liberal) && (m_conservative == other.m_conservative);
+    }
+
+    bool operator!=(const ValueNumPair& other) const
+    {
+        return !(*this == other);
+    }
+
     void operator=(const ValueNumPair& vn2)
     {
         m_liberal      = vn2.m_liberal;