Fix missing zero-offset sequences and add checking (#64805)

SingleAccretion · web-flow · commit 1ea9cf4eb3b4 · 2022-02-18T18:51:51.000+01:00
* Add checking for zero-offset FldSeq addition

* Add a test

* Fix ADDR(LCL_VAR) Zero [FldSeq]

* Always add NotAField field sequences

* NotAField printing improvements
diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -3625,6 +3625,7 @@ class Compiler
     void gtGetArgMsg(GenTreeCall* call, GenTree* arg, unsigned argNum, char* bufp, unsigned bufLength);
     void gtGetLateArgMsg(GenTreeCall* call, GenTree* arg, int argNum, char* bufp, unsigned bufLength);
     void gtDispArgList(GenTreeCall* call, GenTree* lastCallOperand, IndentStack* indentStack);
+    void gtDispAnyFieldSeq(FieldSeqNode* fieldSeq);
     void gtDispFieldSeq(FieldSeqNode* pfsn);
 
     void gtDispRange(LIR::ReadOnlyRange const& range);
@@ -5574,6 +5575,7 @@ class Compiler
 
 #ifdef DEBUG
     void fgDebugCheckExceptionSets();
+    void fgDebugCheckValueNumberedTree(GenTree* tree);
 #endif
 
     // These are the current value number for the memory implicit variables while
diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp
@@ -9178,7 +9178,7 @@ void Compiler::gtDispZeroFieldSeq(GenTree* tree)
         if (map->Lookup(tree, &fldSeq))
         {
             printf(" Zero");
-            gtDispFieldSeq(fldSeq);
+            gtDispAnyFieldSeq(fldSeq);
         }
     }
 }
@@ -10295,9 +10295,28 @@ void Compiler::gtDispConst(GenTree* tree)
     }
 }
 
+//------------------------------------------------------------------------
+// gtDispFieldSeq: "gtDispFieldSeq" that also prints "<NotAField>".
+//
+// Useful for printing zero-offset field sequences.
+//
+void Compiler::gtDispAnyFieldSeq(FieldSeqNode* fieldSeq)
+{
+    if (fieldSeq == FieldSeqStore::NotAField())
+    {
+        printf(" Fseq<NotAField>");
+        return;
+    }
+
+    gtDispFieldSeq(fieldSeq);
+}
+
+//------------------------------------------------------------------------
+// gtDispFieldSeq: Print out the fields in this field sequence.
+//
 void Compiler::gtDispFieldSeq(FieldSeqNode* pfsn)
 {
-    if (pfsn == FieldSeqStore::NotAField() || (pfsn == nullptr))
+    if ((pfsn == nullptr) || (pfsn == FieldSeqStore::NotAField()))
     {
         return;
     }
diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp
@@ -13724,8 +13724,7 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add)
     // TODO-Bug: this code will lose the GC-ness of a tree like "native int + byref(0)".
     if (op2->IsIntegralConst(0) && ((add->TypeGet() == op1->TypeGet()) || !op1->TypeIs(TYP_REF)))
     {
-        if (op2->IsCnsIntOrI() && (op2->AsIntCon()->gtFieldSeq != nullptr) &&
-            (op2->AsIntCon()->gtFieldSeq != FieldSeqStore::NotAField()))
+        if (op2->IsCnsIntOrI() && varTypeIsI(op1))
         {
             fgAddFieldSeqForZeroOffset(op1, op2->AsIntCon()->gtFieldSeq);
         }
@@ -17935,7 +17934,7 @@ void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZ
     if (verbose)
     {
         printf("\nfgAddFieldSeqForZeroOffset for");
-        gtDispFieldSeq(fieldSeqZero);
+        gtDispAnyFieldSeq(fieldSeqZero);
 
         printf("\naddr (Before)\n");
         gtDispNode(addr, nullptr, nullptr, false);
diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp
@@ -4167,32 +4167,33 @@ ValueNum ValueNumStore::VNForFieldSeq(FieldSeqNode* fieldSeq)
     {
         return VNForNull();
     }
-    else if (fieldSeq == FieldSeqStore::NotAField())
+
+    ValueNum fieldSeqVN;
+    if (fieldSeq == FieldSeqStore::NotAField())
     {
         // We always allocate a new, unique VN in this call.
         Chunk*   c                 = GetAllocChunk(TYP_REF, CEA_NotAField);
         unsigned offsetWithinChunk = c->AllocVN();
-        ValueNum result            = c->m_baseVN + offsetWithinChunk;
-        return result;
+        fieldSeqVN                 = c->m_baseVN + offsetWithinChunk;
     }
     else
     {
         ssize_t  fieldHndVal = ssize_t(fieldSeq->m_fieldHnd);
         ValueNum fieldHndVN  = VNForHandle(fieldHndVal, GTF_ICON_FIELD_HDL);
         ValueNum seqNextVN   = VNForFieldSeq(fieldSeq->m_next);
-        ValueNum fieldSeqVN  = VNForFunc(TYP_REF, VNF_FieldSeq, fieldHndVN, seqNextVN);
+        fieldSeqVN           = VNForFunc(TYP_REF, VNF_FieldSeq, fieldHndVN, seqNextVN);
+    }
 
 #ifdef DEBUG
-        if (m_pComp->verbose)
-        {
-            printf("    FieldSeq");
-            vnDump(m_pComp, fieldSeqVN);
-            printf(" is " FMT_VN "\n", fieldSeqVN);
-        }
+    if (m_pComp->verbose)
+    {
+        printf("    FieldSeq");
+        vnDump(m_pComp, fieldSeqVN);
+        printf(" is " FMT_VN "\n", fieldSeqVN);
+    }
 #endif
 
-        return fieldSeqVN;
-    }
+    return fieldSeqVN;
 }
 
 FieldSeqNode* ValueNumStore::FieldSeqVNToFieldSeq(ValueNum vn)
@@ -5961,6 +5962,7 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr)
         switch (funcApp.m_func)
         {
             case VNF_FieldSeq:
+            case VNF_NotAField:
                 vnDumpFieldSeq(comp, &funcApp, true);
                 break;
             case VNF_MapSelect:
@@ -6068,6 +6070,12 @@ 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("<NotAField>");
+        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);
@@ -8592,12 +8600,21 @@ void Compiler::fgValueNumberTree(GenTree* tree)
                         newVN = vnStore->VNForExpr(compCurBB, TYP_BYREF);
                     }
                 }
+
                 if (newVN == ValueNumStore::NoVN)
                 {
+                    // We may have a zero-offset field sequence on this ADDR.
+                    FieldSeqNode* zeroOffsetFieldSeq = nullptr;
+                    if (GetZeroOffsetFieldMap()->Lookup(tree, &zeroOffsetFieldSeq))
+                    {
+                        fieldSeq = GetFieldSeqStore()->Append(fieldSeq, zeroOffsetFieldSeq);
+                    }
+
                     newVN = vnStore->VNForFunc(TYP_BYREF, VNF_PtrToLoc,
                                                vnStore->VNForIntCon(arg->AsLclVarCommon()->GetLclNum()),
                                                vnStore->VNForFieldSeq(fieldSeq));
                 }
+
                 tree->gtVNPair.SetBoth(newVN);
             }
             else if ((arg->gtOper == GT_IND) || arg->OperIsBlk())
@@ -8608,8 +8625,7 @@ void Compiler::fgValueNumberTree(GenTree* tree)
 
                 ValueNumPair  addrVNP            = ValueNumPair();
                 FieldSeqNode* zeroOffsetFieldSeq = nullptr;
-                if (GetZeroOffsetFieldMap()->Lookup(tree, &zeroOffsetFieldSeq) &&
-                    (zeroOffsetFieldSeq != FieldSeqStore::NotAField()))
+                if (GetZeroOffsetFieldMap()->Lookup(tree, &zeroOffsetFieldSeq))
                 {
                     ValueNum addrExtended = vnStore->ExtendPtrVN(arg->AsIndir()->Addr(), zeroOffsetFieldSeq);
                     if (addrExtended != ValueNumStore::NoVN)
@@ -9270,6 +9286,8 @@ void Compiler::fgValueNumberTree(GenTree* tree)
             printf("\n");
         }
     }
+
+    fgDebugCheckValueNumberedTree(tree);
 #endif // DEBUG
 }
 
@@ -10965,6 +10983,72 @@ void Compiler::fgDebugCheckExceptionSets()
     }
 }
 
+//------------------------------------------------------------------------
+// fgDebugCheckValueNumberedTree: Verify proper numbering for "tree".
+//
+// Currently only checks that we have not forgotten to add a zero-offset
+// field sequence to "tree"'s value number.
+//
+// Arguments:
+//    tree - The tree, that has just been numbered, to check
+//
+void Compiler::fgDebugCheckValueNumberedTree(GenTree* tree)
+{
+    FieldSeqNode* zeroOffsetFldSeq;
+    if (GetZeroOffsetFieldMap()->Lookup(tree, &zeroOffsetFldSeq))
+    {
+        // Empty field sequences should never be recorded in the map.
+        assert(zeroOffsetFldSeq != nullptr);
+
+        ValueNum vns[] = {tree->GetVN(VNK_Liberal), tree->GetVN(VNK_Conservative)};
+        for (ValueNum vn : vns)
+        {
+            VNFuncApp vnFunc;
+            if (vnStore->GetVNFunc(vn, &vnFunc))
+            {
+                FieldSeqNode* fullFldSeq;
+                switch (vnFunc.m_func)
+                {
+                    case VNF_PtrToLoc:
+                    case VNF_PtrToStatic:
+                        fullFldSeq = vnStore->FieldSeqVNToFieldSeq(vnFunc.m_args[1]);
+                        break;
+
+                    case VNF_PtrToArrElem:
+                        fullFldSeq = vnStore->FieldSeqVNToFieldSeq(vnFunc.m_args[3]);
+                        break;
+
+                    default:
+                        continue;
+                }
+
+                // Verify that the "fullFldSeq" we have just collected is of the
+                // form "[outer fields, zeroOffsetFldSeq]", or is "NotAField".
+                if (fullFldSeq == FieldSeqStore::NotAField())
+                {
+                    continue;
+                }
+
+                // This check relies on the canonicality of field sequences.
+                FieldSeqNode* fldSeq                = fullFldSeq;
+                bool          zeroOffsetFldSeqFound = false;
+                while (fldSeq != nullptr)
+                {
+                    if (fldSeq == zeroOffsetFldSeq)
+                    {
+                        zeroOffsetFldSeqFound = true;
+                        break;
+                    }
+
+                    fldSeq = fldSeq->m_next;
+                }
+
+                assert(zeroOffsetFldSeqFound);
+            }
+        }
+    }
+}
+
 // This method asserts that SSA name constraints specified are satisfied.
 // Until we figure out otherwise, all VN's are assumed to be liberal.
 // TODO-Cleanup: new JitTestLabels for lib vs cons vs both VN classes?
diff --git a/src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.cs b/src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.cs
@@ -0,0 +1,102 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+using System.Runtime.CompilerServices;
+
+class ZeroOffsetFieldSeqs
+{
+    private static UnionStruct s_union;
+
+    public static int Main()
+    {
+        if (ProblemWithArrayUnions(new UnionStruct[] { default }))
+        {
+            return 101;
+        }
+
+        if (ProblemWithStaticUnions())
+        {
+            return 102;
+        }
+
+        if (AnotherProblemWithArrayUnions(new UnionStruct[] { default }))
+        {
+            return 103;
+        }
+
+        return 100;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static bool ProblemWithArrayUnions(UnionStruct[] a)
+    {
+        if (a[0].UnionOne.UnionOneFldTwo == 0)
+        {
+            a[0].UnionTwo.UnionTwoFldTwo = 1;
+            if (a[0].UnionOne.UnionOneFldTwo == 0)
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static bool ProblemWithStaticUnions()
+    {
+        if (s_union.UnionOne.UnionOneFldTwo == 0)
+        {
+            s_union.UnionTwo.UnionTwoFldTwo = 1;
+            if (s_union.UnionOne.UnionOneFldTwo == 0)
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static bool AnotherProblemWithArrayUnions(UnionStruct[] a)
+    {
+        ref var p1 = ref a[0];
+        ref var p1a = ref Unsafe.Add(ref p1, 0).UnionOne;
+        ref var p1b = ref Unsafe.Add(ref p1, 0).UnionTwo;
+
+        if (p1a.UnionOneFldTwo == 0)
+        {
+            p1b.UnionTwoFldTwo = 1;
+            if (p1a.UnionOneFldTwo == 0)
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+}
+
+[StructLayout(LayoutKind.Explicit)]
+struct UnionStruct
+{
+    [FieldOffset(0)]
+    public UnionPartOne UnionOne;
+    [FieldOffset(0)]
+    public UnionPartTwo UnionTwo;
+}
+
+struct UnionPartOne
+{
+    public long UnionOneFldOne;
+    public long UnionOneFldTwo;
+}
+
+struct UnionPartTwo
+{
+    public long UnionTwoFldOne;
+    public long UnionTwoFldTwo;
+}
+
diff --git a/src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.csproj b/src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.csproj
@@ -0,0 +1,12 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>

Original file line number	Diff line number	Diff line change
`@@ -9178,7 +9178,7 @@ void Compiler::gtDispZeroFieldSeq(GenTree* tree)`
`9178`	`9178`	`if (map->Lookup(tree, &fldSeq))`
`9179`	`9179`	`{`
`9180`	`9180`	`printf(" Zero");`
`9181`		`- gtDispFieldSeq(fldSeq);`
	`9181`	`+ gtDispAnyFieldSeq(fldSeq);`
`9182`	`9182`	`}`
`9183`	`9183`	`}`
`9184`	`9184`	`}`
`@@ -10295,9 +10295,28 @@ void Compiler::gtDispConst(GenTree* tree)`
`10295`	`10295`	`}`
`10296`	`10296`	`}`
`10297`	`10297`
	`10298`	`+//------------------------------------------------------------------------`
	`10299`	`+// gtDispFieldSeq: "gtDispFieldSeq" that also prints "<NotAField>".`
	`10300`	`+//`
	`10301`	`+// Useful for printing zero-offset field sequences.`
	`10302`	`+//`
	`10303`	`+void Compiler::gtDispAnyFieldSeq(FieldSeqNode* fieldSeq)`
	`10304`	`+{`
	`10305`	`+ if (fieldSeq == FieldSeqStore::NotAField())`
	`10306`	`+ {`
	`10307`	`+ printf(" Fseq<NotAField>");`
	`10308`	`+ return;`
	`10309`	`+ }`
	`10310`	`+`
	`10311`	`+ gtDispFieldSeq(fieldSeq);`
	`10312`	`+}`
	`10313`	`+`
	`10314`	`+//------------------------------------------------------------------------`
	`10315`	`+// gtDispFieldSeq: Print out the fields in this field sequence.`
	`10316`	`+//`
`10298`	`10317`	`void Compiler::gtDispFieldSeq(FieldSeqNode* pfsn)`
`10299`	`10318`	`{`
`10300`		`- if (pfsn == FieldSeqStore::NotAField() \|\| (pfsn == nullptr))`
	`10319`	`+ if ((pfsn == nullptr) \|\| (pfsn == FieldSeqStore::NotAField()))`
`10301`	`10320`	`{`
`10302`	`10321`	`return;`
`10303`	`10322`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13724,8 +13724,7 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add)`
`13724`	`13724`	`// TODO-Bug: this code will lose the GC-ness of a tree like "native int + byref(0)".`
`13725`	`13725`	`if (op2->IsIntegralConst(0) && ((add->TypeGet() == op1->TypeGet()) \|\| !op1->TypeIs(TYP_REF)))`
`13726`	`13726`	`{`
`13727`		`- if (op2->IsCnsIntOrI() && (op2->AsIntCon()->gtFieldSeq != nullptr) &&`
`13728`		`- (op2->AsIntCon()->gtFieldSeq != FieldSeqStore::NotAField()))`
	`13727`	`+ if (op2->IsCnsIntOrI() && varTypeIsI(op1))`
`13729`	`13728`	`{`
`13730`	`13729`	`fgAddFieldSeqForZeroOffset(op1, op2->AsIntCon()->gtFieldSeq);`
`13731`	`13730`	`}`
`@@ -17935,7 +17934,7 @@ void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZ`
`17935`	`17934`	`if (verbose)`
`17936`	`17935`	`{`
`17937`	`17936`	`printf("\nfgAddFieldSeqForZeroOffset for");`
`17938`		`- gtDispFieldSeq(fieldSeqZero);`
	`17937`	`+ gtDispAnyFieldSeq(fieldSeqZero);`
`17939`	`17938`
`17940`	`17939`	`printf("\naddr (Before)\n");`
`17941`	`17940`	`gtDispNode(addr, nullptr, nullptr, false);`