From af78980296a6741253fa48d3501a74637084a704 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 4 Feb 2022 18:44:19 +0300 Subject: [PATCH 1/5] Add checking for zero-offset FldSeq addition --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/valuenum.cpp | 68 ++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 281758d67ef88c..753329951a50fa 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5573,6 +5573,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/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 8d0290cd4f36df..4f0e5515e21a12 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9270,6 +9270,8 @@ void Compiler::fgValueNumberTree(GenTree* tree) printf("\n"); } } + + fgDebugCheckValueNumberedTree(tree); #endif // DEBUG } @@ -10965,6 +10967,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? From 7a29457d204a416f0ebf2fdccd8f0bffb6e815f7 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 4 Feb 2022 16:33:06 +0300 Subject: [PATCH 2/5] Add a test --- .../opt/ValueNumbering/ZeroOffsetFieldSeqs.cs | 102 ++++++++++++++++++ .../ValueNumbering/ZeroOffsetFieldSeqs.csproj | 12 +++ 2 files changed, 114 insertions(+) create mode 100644 src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.cs create mode 100644 src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.csproj diff --git a/src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.cs b/src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.cs new file mode 100644 index 00000000000000..3ef8c35aba6e6a --- /dev/null +++ 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 new file mode 100644 index 00000000000000..f3e1cbd44b4041 --- /dev/null +++ b/src/tests/JIT/opt/ValueNumbering/ZeroOffsetFieldSeqs.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + From 1be22fd9f9a641451ce0a20844d58d7279b621b0 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 4 Feb 2022 16:46:13 +0300 Subject: [PATCH 3/5] Fix ADDR(LCL_VAR) Zero [FldSeq] --- src/coreclr/jit/valuenum.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 4f0e5515e21a12..85aea7fc73ed8e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8592,12 +8592,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()) From bbcedae8617cee14f2111f6eecf277bd53cfd9b2 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sat, 5 Feb 2022 15:25:23 +0300 Subject: [PATCH 4/5] Always add NotAField field sequences --- src/coreclr/jit/morph.cpp | 3 +-- src/coreclr/jit/valuenum.cpp | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 414b82f5480c55..2e62516c84b692 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13641,8 +13641,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); } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 85aea7fc73ed8e..8e2466594f016e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8617,8 +8617,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) From 7ce21ab13b7772ff8e1e501bcfb326dd9da4f21c Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sat, 5 Feb 2022 15:43:48 +0300 Subject: [PATCH 5/5] NotAField printing improvements --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/gentree.cpp | 23 +++++++++++++++++++++-- src/coreclr/jit/morph.cpp | 2 +- src/coreclr/jit/valuenum.cpp | 32 ++++++++++++++++++++------------ 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 753329951a50fa..db90b4962dd10f 100644 --- 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); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 8e1dd02c2c2f24..3f73e2ce7c4042 100644 --- 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 "". +// +// Useful for printing zero-offset field sequences. +// +void Compiler::gtDispAnyFieldSeq(FieldSeqNode* fieldSeq) +{ + if (fieldSeq == FieldSeqStore::NotAField()) + { + printf(" Fseq"); + 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 index 2e62516c84b692..3bb62970a854f0 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -17844,7 +17844,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 index 8e2466594f016e..dd5ec9ad6b7f67 100644 --- 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(""); + 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);