diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 90ac6bbe1258f4..44ee64dfcd8d69 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -10506,6 +10506,13 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, __in __in_z _ case GT_LCL_FLD_ADDR: case GT_STORE_LCL_FLD: case GT_STORE_LCL_VAR: + + if (tree->gtFlags & GTF_VAR_DONT_VN) + { + printf("$"); + --msgLength; + } + if (tree->gtFlags & GTF_VAR_USEASG) { printf("U"); @@ -11360,7 +11367,7 @@ void Compiler::gtDispFieldSeq(FieldSeqNode* pfsn) } else { - printf("%s", eeGetFieldName(fldHnd)); + printf("%s(0x%x)", eeGetFieldName(fldHnd), fldHnd); } pfsn = pfsn->m_next; if (pfsn != nullptr) @@ -12042,23 +12049,25 @@ void Compiler::gtDispTree(GenTree* tree, break; case GT_FIELD: - if (FieldSeqStore::IsPseudoField(tree->AsField()->gtFldHnd)) + { + GenTreeField* fld = tree->AsField(); + if (FieldSeqStore::IsPseudoField(fld->gtFldHnd)) { - printf(" #PseudoField:0x%x", tree->AsField()->gtFldOffset); + printf(" #PseudoField:0x%x", fld->gtFldOffset); } else { - printf(" %s", eeGetFieldName(tree->AsField()->gtFldHnd), 0); + printf(" %s(0x%x)", eeGetFieldName(fld->gtFldHnd), fld->gtFldHnd); } - gtDispCommonEndLine(tree); + gtDispCommonEndLine(fld); - if (tree->AsField()->gtFldObj && !topOnly) + if (fld->gtFldObj && !topOnly) { - gtDispChild(tree->AsField()->gtFldObj, indentStack, IIArcBottom); + gtDispChild(fld->gtFldObj, indentStack, IIArcBottom); } - break; + } case GT_CALL: { diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 2c8ad35b8cdb0f..43ca9aa62b2ae7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -467,6 +467,10 @@ enum GenTreeFlags : unsigned int // This flag identifies such nodes in order to make sure that fgDoNormalizeOnStore() is called // on their parents in post-order morph. // Relevant for inlining optimizations (see fgInlinePrependStatements) + GTF_VAR_DONT_VN = 0x00080000, // GT_LCL_VAR -- it is a special tree, usually a result of Unsafe.As, + // that should generate a unique VN pair because Value numbering does not understand it. + // This is a temporary fix for 6.0, a proper fix would be to support Obj casts as a special node, + // support struct LCL_FLD and improve VN to catch and assert on CORINFO_CLASS_HANDLE mistmatch. GTF_VAR_ARR_INDEX = 0x00000020, // The variable is part of (the index portion of) an array index expression. // Shares a value with GTF_REVERSE_OPS, which is meaningless for local var. @@ -3526,6 +3530,16 @@ struct GenTreeLclVar : public GenTreeLclVarCommon assert(OperIsLocal(oper) || OperIsLocalAddr(oper)); } + void SetDontVN() + { + gtFlags |= GTF_VAR_DONT_VN; + } + + bool DontVN() const + { + return (gtFlags & GTF_VAR_DONT_VN) != 0; + } + #if DEBUGGABLE_GENTREE GenTreeLclVar() : GenTreeLclVarCommon() { diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index da396f145bfe05..5898c4a07c42a6 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -17284,9 +17284,10 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) GenTree* addrChild = effectiveRetVal->gtGetOp1(); if (addrChild->OperGet() == GT_LCL_VAR) { - LclVarDsc* varDsc = lvaGetDesc(addrChild->AsLclVarCommon()); + GenTreeLclVar* lclVar = addrChild->AsLclVar(); + LclVarDsc* varDsc = lvaGetDesc(lclVar); - if (varTypeIsStruct(addrChild->TypeGet()) && !isOpaqueSIMDLclVar(varDsc)) + if (varTypeIsStruct(lclVar->TypeGet()) && !isOpaqueSIMDLclVar(varDsc)) { CORINFO_CLASS_HANDLE referentClassHandle; CorInfoType referentType = @@ -17300,15 +17301,13 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode) // to struct2. struct1 and struct2 are different so we are "reinterpreting" the struct. // This may happen in, for example, System.Runtime.CompilerServices.Unsafe.As. - // We need to mark the source struct variable as having overlapping fields because its - // fields may be accessed using field handles of a different type, which may confuse - // optimizations, in particular, value numbering. + // We need to exclude the local from VN because VN optimizations don't expect such + // transformations. - JITDUMP("\nSetting lvOverlappingFields to true on V%02u because of struct " - "reinterpretation\n", - addrChild->AsLclVarCommon()->GetLclNum()); + JITDUMP("Setting DontVN() on [%06u] because of struct reinterpretation.\n", + dspTreeID(lclVar)); - varDsc->lvOverlappingFields = true; + lclVar->SetDontVN(); } } } diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index caa8a1bdc872d6..e06be5463ff03b 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -39,6 +39,10 @@ class LocalAddressVisitor final : public GenTreeVisitor unsigned m_lclNum; unsigned m_offset; bool m_address; + bool m_excludeFromVN; // This value should not participate in VN optimizations, + // for example, because it represents a reinterpreted struct. + // When we transform it we should set `DontVN()` for `LclVar` + // and `NotAField` for `LclFld`. INDEBUG(bool m_consumed;) public: @@ -49,6 +53,7 @@ class LocalAddressVisitor final : public GenTreeVisitor , m_lclNum(BAD_VAR_NUM) , m_offset(0) , m_address(false) + , m_excludeFromVN(false) #ifdef DEBUG , m_consumed(false) #endif // DEBUG @@ -112,6 +117,10 @@ class LocalAddressVisitor final : public GenTreeVisitor assert(!IsLocation() && !IsAddress()); m_lclNum = lclVar->GetLclNum(); + if (lclVar->DontVN()) + { + m_excludeFromVN = true; + } assert(m_offset == 0); assert(m_fieldSeq == nullptr); @@ -195,10 +204,11 @@ class LocalAddressVisitor final : public GenTreeVisitor if (val.IsLocation()) { - m_address = true; - m_lclNum = val.m_lclNum; - m_offset = val.m_offset; - m_fieldSeq = val.m_fieldSeq; + m_address = true; + m_lclNum = val.m_lclNum; + m_offset = val.m_offset; + m_fieldSeq = val.m_fieldSeq; + m_excludeFromVN = val.m_excludeFromVN; } INDEBUG(val.Consume();) @@ -246,7 +256,7 @@ class LocalAddressVisitor final : public GenTreeVisitor m_lclNum = val.m_lclNum; m_offset = newOffset.Value(); - if (field->gtFldMayOverlap) + if (field->gtFldMayOverlap || val.m_excludeFromVN) { m_fieldSeq = FieldSeqStore::NotAField(); } @@ -287,9 +297,10 @@ class LocalAddressVisitor final : public GenTreeVisitor if (val.IsAddress()) { - m_lclNum = val.m_lclNum; - m_offset = val.m_offset; - m_fieldSeq = val.m_fieldSeq; + m_lclNum = val.m_lclNum; + m_offset = val.m_offset; + m_fieldSeq = val.m_fieldSeq; + m_excludeFromVN = val.m_excludeFromVN; } INDEBUG(val.Consume();) @@ -942,6 +953,8 @@ class LocalAddressVisitor final : public GenTreeVisitor ClassLayout* structLayout = nullptr; FieldSeqNode* fieldSeq = val.FieldSeq(); + bool cantDoVNOnTHisTree = false; + if ((fieldSeq != nullptr) && (fieldSeq != FieldSeqStore::NotAField())) { // TODO-ADDR: ObjectAllocator produces FIELD nodes with FirstElemPseudoField as field @@ -1013,19 +1026,28 @@ class LocalAddressVisitor final : public GenTreeVisitor structLayout = indir->AsBlk()->GetLayout(); } - // We're not going to produce a TYP_STRUCT LCL_FLD so we don't need the field sequence. - fieldSeq = nullptr; + if (fieldSeq != nullptr) + { + // We're not going to produce a TYP_STRUCT LCL_FLD so we don't need the field sequence. + fieldSeq = nullptr; + JITDUMP("Dropped field seq from [%06u]\n", m_compiler->dspTreeID(indir)); + } } // We're only processing TYP_STRUCT variables now so the layout should never be null, // otherwise the below layout equality check would be insufficient. assert(varDsc->GetLayout() != nullptr); - if ((val.Offset() == 0) && (structLayout != nullptr) && + if ((val.Offset() == 0) && (structLayout != nullptr) && (fieldSeq == nullptr) && ClassLayout::AreCompatible(structLayout, varDsc->GetLayout())) { indir->ChangeOper(GT_LCL_VAR); indir->AsLclVar()->SetLclNum(val.LclNum()); + + if (structLayout->GetClassHandle() != varDsc->GetLayout()->GetClassHandle()) + { + cantDoVNOnTHisTree = true; + } } else if (!varTypeIsStruct(indir->TypeGet())) { @@ -1065,6 +1087,14 @@ class LocalAddressVisitor final : public GenTreeVisitor indir->gtFlags = flags; + if (cantDoVNOnTHisTree) + { + assert(indir->OperIs(GT_LCL_VAR)); + indir->AsLclVar()->SetDontVN(); + JITDUMP("Exclude local var tree [%06u] from VN because because of struct reinterpretation\n", + m_compiler->dspTreeID(indir)); + } + INDEBUG(m_stmtModified = true;) } diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index d4033efa2b2c04..d6269af5dc1f98 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7172,7 +7172,10 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) rhsVarDsc = &lvaTable[rhsLclNum]; if (!lvaInSsa(rhsLclNum) || rhsFldSeq == FieldSeqStore::NotAField()) { - rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, rhsLclVarTree->TypeGet())); + isNewUniq = true; + } + else if (rhsLclVarTree->OperIs(GT_LCL_VAR) && rhsLclVarTree->AsLclVar()->DontVN()) + { isNewUniq = true; } else @@ -7187,7 +7190,6 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) } else { - rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, rhs->TypeGet())); isNewUniq = true; } } @@ -7272,6 +7274,11 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) JITDUMP(" *** Missing field sequence info for Dst/LHS of COPYBLK\n"); isNewUniq = true; } + else if (lclVarTree->OperIs(GT_LCL_VAR) && lclVarTree->AsLclVar()->DontVN()) + { + JITDUMP(" ***Struct reinterpretation on rhs of COPYBLK\n"); + isNewUniq = true; + } if (isNewUniq) { @@ -7399,9 +7406,9 @@ void Compiler::fgValueNumberTree(GenTree* tree) case GT_LCL_VAR: { - GenTreeLclVarCommon* lcl = tree->AsLclVarCommon(); - unsigned lclNum = lcl->GetLclNum(); - LclVarDsc* varDsc = &lvaTable[lclNum]; + GenTreeLclVar* lcl = tree->AsLclVar(); + unsigned lclNum = lcl->GetLclNum(); + LclVarDsc* varDsc = &lvaTable[lclNum]; if (varDsc->CanBeReplacedWithItsField(this)) { @@ -7457,7 +7464,11 @@ void Compiler::fgValueNumberTree(GenTree* tree) unsigned typSize = genTypeSize(genActualType(typ)); unsigned varSize = genTypeSize(genActualType(varDsc->TypeGet())); - if (typSize == varSize) + if (lcl->DontVN()) + { + generateUniqueVN = true; + } + else if (typSize == varSize) { lcl->gtVNPair = wholeLclVarVNP; } @@ -7585,7 +7596,16 @@ void Compiler::fgValueNumberTree(GenTree* tree) else { ValueNumPair lclVNPair = varDsc->GetPerSsaData(ssaNum)->m_vnPair; - tree->gtVNPair = vnStore->VNPairApplySelectors(lclVNPair, lclFld->GetFieldSeq(), indType); + if (lclVNPair.GetLiberal() == ValueNumStore::NoVN) + { + // We didn't not assign a correct VN to the local, probably it was written using a different + // type. + tree->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, indType)); + } + else + { + tree->gtVNPair = vnStore->VNPairApplySelectors(lclVNPair, lclFld->GetFieldSeq(), indType); + } } } } @@ -7782,8 +7802,8 @@ void Compiler::fgValueNumberTree(GenTree* tree) { case GT_LCL_VAR: { - GenTreeLclVarCommon* lcl = lhs->AsLclVarCommon(); - unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lcl); + GenTreeLclVar* lcl = lhs->AsLclVar(); + unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lcl); // Should not have been recorded as updating the GC heap. assert(!GetMemorySsaMap(GcHeap)->Lookup(tree, &memorySsaNum)); @@ -7795,6 +7815,18 @@ void Compiler::fgValueNumberTree(GenTree* tree) assert(rhsVNPair.GetLiberal() != ValueNumStore::NoVN); + ValueNumPair lhsVNPair; + + if (!lcl->DontVN()) + { + lhsVNPair = rhsVNPair; + } + else + { + ValueNum uniqVN = vnStore->VNForExpr(compCurBB, lcl->TypeGet()); + lhsVNPair.SetBoth(uniqVN); + } + lhs->gtVNPair = rhsVNPair; lvaTable[lcl->GetLclNum()].GetPerSsaData(lclDefSsaNum)->m_vnPair = rhsVNPair; diff --git a/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs b/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs index 656918e5fe6e7d..b1a6c293aef32c 100644 --- a/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs +++ b/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs @@ -65,7 +65,6 @@ public void Clear() } [Fact] - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/42517", RuntimeConfiguration.Checked)] public void Advance() { { diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_54102/Runtime_54102.cs b/src/tests/JIT/Regression/JitBlue/Runtime_54102/Runtime_54102.cs new file mode 100644 index 00000000000000..8afef4323c2fbd --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_54102/Runtime_54102.cs @@ -0,0 +1,52 @@ +// 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.CompilerServices; + +// Generated by Fuzzlyn v1.1 on 2021-06-12 11:06:35 +// Seed: 8276490119048877745 +// Reduced from 962.5 KiB to 0.6 KiB in 00:05:32 +// Debug: Outputs 0 +// Release: Outputs 1 + +struct S0 +{ + public ushort F1; + public uint F2; + public byte F3; + public int F5; + public S0(byte f3): this() + { + F3 = f3; + } +} + +struct S1 +{ + public S0 F0; + public S0 F4; + public S1(S0 f0): this() + { + F0 = f0; + } +} + +struct S2 +{ + public S1 F0; + public S2(S1 f0): this() + { + F0 = f0; + } +} + +public class Program +{ + public static int Main() + { + S2 vr0 = new S2(new S1(new S0(100))); + int ret = vr0.F0.F0.F3 + vr0.F0.F4.F1; + return ret; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_54102/Runtime_54102.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_54102/Runtime_54102.csproj new file mode 100644 index 00000000000000..f3e1cbd44b4041 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_54102/Runtime_54102.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_56980/Runtime_56980.cs b/src/tests/JIT/Regression/JitBlue/Runtime_56980/Runtime_56980.cs new file mode 100644 index 00000000000000..1ff1c7611e84fc --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_56980/Runtime_56980.cs @@ -0,0 +1,61 @@ +// 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.CompilerServices; + +// Generated by Fuzzlyn v1.2 on 2021-07-06 09:46:44 +// Seed: 16635934940619066544 +// Reduced from 447.5 KiB to 0.6 KiB in 00:02:24 +// Debug: Runs successfully +// Release: Throws 'System.NullReferenceException' +struct S0 +{ + public uint F1; + public byte F3; + public long F4; + public uint F5; + public S0(long f4): this() + { + F4 = f4; + } +} + +class C0 +{ + public S0 F4; +} + +struct S1 +{ + public C0 F2; + public S0 F8; + public S1(C0 f2, S0 f8): this() + { + F2 = f2; + F8 = f8; + } +} + +struct S2 +{ + public S1 F0; + public S2(S1 f0): this() + { + F0 = f0; + } +} + +public class Program +{ + public static int Main() + { + S2 vr0 = new S2(new S1(new C0(), new S0(0))); + M17(ref vr0.F0.F2.F4.F1); + return 100; + } + + static void M17(ref uint arg2) + { + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_56980/Runtime_56980.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_56980/Runtime_56980.csproj new file mode 100644 index 00000000000000..f3e1cbd44b4041 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_56980/Runtime_56980.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + diff --git a/src/tests/JIT/opt/Structs/structcopies.csproj b/src/tests/JIT/opt/Structs/structcopies.csproj index c85bacc9218e5c..f3e1cbd44b4041 100644 --- a/src/tests/JIT/opt/Structs/structcopies.csproj +++ b/src/tests/JIT/opt/Structs/structcopies.csproj @@ -5,8 +5,6 @@ None True - - True