From 5cb7245c32da700f528b3da0082888babac3923a Mon Sep 17 00:00:00 2001 From: Sergey Date: Thu, 5 Aug 2021 12:44:07 -0700 Subject: [PATCH] Avoid VN bugs with struct reinterpetation. --- src/coreclr/jit/gentree.cpp | 7 +++++++ src/coreclr/jit/gentree.h | 14 +++++++++++++ src/coreclr/jit/lclmorph.cpp | 25 ++++++++++++++++++++--- src/coreclr/jit/valuenum.cpp | 39 ++++++++++++++++++++++++++++-------- 4 files changed, 74 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 90ac6bbe1258f..625df15bb5292 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"); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 2c8ad35b8cdb0..43ca9aa62b2ae 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/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index caa8a1bdc872d..4d83faa9443e7 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -942,6 +942,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 +1015,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 +1076,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 d4033efa2b2c0..6c973c42f2937 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; } @@ -7782,8 +7793,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 +7806,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;