diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 281758d67ef88c..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); @@ -5573,6 +5574,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 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 414b82f5480c55..3bb62970a854f0 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); } @@ -17845,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 8d0290cd4f36df..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); @@ -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 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 + + + + +