Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid VN bugs with struct reinterpretation. #57076

Closed
wants to merge 5 commits into from
Closed

Avoid VN bugs with struct reinterpretation. #57076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a5aae3
Add/return tests.
Aug 10, 2021
5cb7245
Avoid VN bugs with struct reinterpetation.
Aug 5, 2021
2af4408
Fix for the fields.
Aug 10, 2021
858202a
add gtFldHnd dumping
Aug 12, 2021
40efb0e
fix jitstress assert
Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
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 Expand Up @@ -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)
Expand Down Expand Up @@ -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:
{
Expand Down
14 changes: 14 additions & 0 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
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
17 changes: 8 additions & 9 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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<TFrom,
// TTo>.
// 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();
}
}
}
Expand Down
52 changes: 41 additions & 11 deletions src/coreclr/jit/lclmorph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
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:
Expand All @@ -49,6 +53,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
, m_lclNum(BAD_VAR_NUM)
, m_offset(0)
, m_address(false)
, m_excludeFromVN(false)
#ifdef DEBUG
, m_consumed(false)
#endif // DEBUG
Expand Down Expand Up @@ -112,6 +117,10 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
assert(!IsLocation() && !IsAddress());

m_lclNum = lclVar->GetLclNum();
if (lclVar->DontVN())
{
m_excludeFromVN = true;
}

assert(m_offset == 0);
assert(m_fieldSeq == nullptr);
Expand Down Expand Up @@ -195,10 +204,11 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>

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();)
Expand Down Expand Up @@ -246,7 +256,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
m_lclNum = val.m_lclNum;
m_offset = newOffset.Value();

if (field->gtFldMayOverlap)
if (field->gtFldMayOverlap || val.m_excludeFromVN)
{
m_fieldSeq = FieldSeqStore::NotAField();
}
Expand Down Expand Up @@ -287,9 +297,10 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>

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();)
Expand Down Expand Up @@ -942,6 +953,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 +1026,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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here, about the Class handle mismatch, and how/why we handle it

{
cantDoVNOnTHisTree = true;
}
}
else if (!varTypeIsStruct(indir->TypeGet()))
{
Expand Down Expand Up @@ -1065,6 +1087,14 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>

indir->gtFlags = flags;

if (cantDoVNOnTHisTree)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here as well

{
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
50 changes: 41 additions & 9 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
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 @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@briansull I am trying to pass the ci, could you please help me with it? The failures look like https://dev.azure.com/dnceng/public/_build/results?buildId=1289712&view=ms.vss-test-web.build-test-results-tab&runId=38073760&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab&resultId=107788
on arm32 windows/linux only with JitStess and only on one test. We assert when try to ApplySelector from NoVN.
I have added this code for one cases but we have more failures, what I don't understand is why we don't hit these asserts without my changes, do I understand correctly that:

  1. LCL_VAR can have a unique VN before the changes;
  2. other code parts expect it and do the right things with it;

if both are true why do we hit these asserts and what is the best way to fix it? Should I add similar checks if (lclVNPair.GetLiberal() == ValueNumStore::NoVN) to all failing cases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand your issue.
It is difficult to answer your questions without a full understanding of what is going on here.

Copy link
Contributor

@briansull briansull Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally it is an error to have read with a ValueNumber of NoVN

Check if line 7500 (above) applies here:

                        // There are a couple of cases where we haven't assigned a valid value number to 'lcl'
                        //
                        if (lcl->gtVNPair.GetLiberal() == ValueNumStore::NoVN)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I don't understand where here do we update lvaTable[lclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair.SetBoth(lclVarVN);?
And when I do lvaTable[lclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair.SetBoth(ValueNumStore::NoVN); it starts to produce errors, so what value should I write to GetPerSsaData when the local has invalid VN and why don't we write it there in the example on line 7500?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandreenko
I can look at a Jit Dump if you have one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you need to mark the local variable as unsafe to ValueNumber

Copy link
Contributor

@briansull briansull Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another fix would be to set

lvaTable[V09].GetPerSsaData() to $500

when processing [000365]

LCL_VAR long V15 tmp9 u:2 (last use) $500

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly VN that we print on LHS of an assignment does not matter at all, the real value is saved in lvaTable[V09].GetPerSsaData() .
At least it is how I understand this:

runtime/src/coreclr/jit/valuenum.cpp

Line 7549 in 7b0a27e

lcl->gtVNPair = ValueNumPair(); // Avoid confusion -- we don't set the VN of a lcl being defined.

but in our case it is a Def, because it is a use:

N005 ( 13, 11) [000365] -A------R----             *  ASG       long   $VN.Void
N004 (  6,  5) [000363] *------N-----             +--*  IND       long   $500
N003 (  3,  3) [000321] -------------             |  \--*  ADDR      byref  Zero Fseq[_00(0xab45adc)] $345
N002 (  3,  2) [000322] $U------N-----            |     \--*  LCL_VAR   struct<System.Runtime.Intrinsics.Vector128`1[Double], 16> V09 tmp3         ud:2->3 $482 <- marked as use.
N001 (  3,  2) [000364] -------------             \--*  LCL_VAR   long   V15 tmp9         u:2 (last use) $500

but, obviously, we don't care about its value, we won't read it anywhere.

What we care about is value of lvaTable[V09].GetPerSsaData() we want it be NoVN and we want the next tree that uses it not to try to optimize it or "see" the values of any of the fields inside this lclVar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you are saying above does sound correct to me. I haven't made many (or any) changes in this area SSA and VN, so it is also my understanding from reading the code.

Copy link
Contributor

@briansull briansull Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But with this approach you will have to check for this value for every read (use) site, because the mismatch can occur either at the def or the use. In the above case we have a mismatch on the def site, which I guess already does write a NoVN, but later at the use site we pull out the NoVN and try to use it in VNPairApplySelectors and the use site won't have the GTF_VAR_DONT_VN flag set as the isn't a mismatch on the use site.

// type.
tree->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, indType));
}
else
{
tree->gtVNPair = vnStore->VNPairApplySelectors(lclVNPair, lclFld->GetFieldSeq(), indType);
}
}
}
}
Expand Down Expand Up @@ -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));
Expand All @@ -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);
}

Comment on lines +7818 to +7829
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears lhsVNPair is unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, thanks for the catch. I need to investigate why it fixes one of the test cases that I was targeting with this change.

lhs->gtVNPair = rhsVNPair;
lvaTable[lcl->GetLclNum()].GetPerSsaData(lclDefSsaNum)->m_vnPair = rhsVNPair;

Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ public void Clear()
}

[Fact]
[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/42517", RuntimeConfiguration.Checked)]
public void Advance()
{
{
Expand Down
Loading