Skip to content

Commit

Permalink
Avoid VN bugs with struct reinterpetation.
Browse files Browse the repository at this point in the history
  • Loading branch information
Sergey committed Aug 11, 2021
1 parent 3a5aae3 commit 5cb7245
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 11 deletions.
7 changes: 7 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
14 changes: 14 additions & 0 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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()
{
Expand Down
25 changes: 22 additions & 3 deletions src/coreclr/jit/lclmorph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,8 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
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
Expand Down Expand Up @@ -1013,19 +1015,28 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
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()))
{
Expand Down Expand Up @@ -1065,6 +1076,14 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>

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;)
}

Expand Down
39 changes: 31 additions & 8 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -7187,7 +7190,6 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
}
else
{
rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, rhs->TypeGet()));
isNewUniq = true;
}
}
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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))
{
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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));
Expand All @@ -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;

Expand Down

0 comments on commit 5cb7245

Please sign in to comment.