Skip to content

Add support for TYP_BYREF LCL_FLDs to VN #64501

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

Merged
jakobbotsch merged 3 commits into from
Feb 6, 2022
Merged

Add support for TYP_BYREF LCL_FLDs to VN #64501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
022c47b
Do not add a zero-offset FldSeq to LCL_FLD directly
SingleAccretion Jan 28, 2022
981d37f
Fix the same issue in "ChangeOper"
SingleAccretion Jan 29, 2022
c93785c
Read zero-offset FldSeqs on LclFld in VN
SingleAccretion Jan 29, 2022
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
59 changes: 21 additions & 38 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1321,24 +1321,33 @@ inline void GenTree::SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
SetVtableForOper(oper);
#endif // DEBUGGABLE_GENTREE

if (oper == GT_CNS_INT)
if (vnUpdate == CLEAR_VN)
{
AsIntCon()->gtFieldSeq = nullptr;
// Clear the ValueNum field as well.
gtVNPair.SetBoth(ValueNumStore::NoVN);
}

#if defined(TARGET_ARM)
if (oper == GT_MUL_LONG)
// Do "oper"-specific initializations. TODO-Cleanup: these are too ad-hoc to be reliable.
// The bashing code should decide itself what to initialize and what to leave as it was.
switch (oper)
{
// We sometimes bash GT_MUL to GT_MUL_LONG, which converts it from GenTreeOp to GenTreeMultiRegOp.
AsMultiRegOp()->gtOtherReg = REG_NA;
AsMultiRegOp()->ClearOtherRegFlags();
}
case GT_CNS_INT:
AsIntCon()->gtFieldSeq = FieldSeqStore::NotAField();
break;
#if defined(TARGET_ARM)
case GT_MUL_LONG:
// We sometimes bash GT_MUL to GT_MUL_LONG, which converts it from GenTreeOp to GenTreeMultiRegOp.
AsMultiRegOp()->gtOtherReg = REG_NA;
AsMultiRegOp()->ClearOtherRegFlags();
break;
#endif
case GT_LCL_FLD:
AsLclFld()->SetLclOffs(0);
AsLclFld()->SetFieldSeq(FieldSeqStore::NotAField());
break;

if (vnUpdate == CLEAR_VN)
{
// Clear the ValueNum field as well.
gtVNPair.SetBoth(ValueNumStore::NoVN);
default:
break;
}
}

Expand Down Expand Up @@ -1418,32 +1427,6 @@ inline void GenTree::ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
}
SetOper(oper, vnUpdate);
gtFlags &= mask;

// Do "oper"-specific initializations...
switch (oper)
{
case GT_LCL_FLD:
{
// The original GT_LCL_VAR might be annotated with a zeroOffset field.
FieldSeqNode* zeroFieldSeq = nullptr;
Compiler* compiler = JitTls::GetCompiler();
bool isZeroOffset = compiler->GetZeroOffsetFieldMap()->Lookup(this, &zeroFieldSeq);

AsLclFld()->SetLclOffs(0);
AsLclFld()->SetFieldSeq(FieldSeqStore::NotAField());

if (zeroFieldSeq != nullptr)
{
// Set the zeroFieldSeq in the GT_LCL_FLD node
AsLclFld()->SetFieldSeq(zeroFieldSeq);
// and remove the annotation from the ZeroOffsetFieldMap
compiler->GetZeroOffsetFieldMap()->Remove(this);
}
break;
}
default:
break;
}
}

inline void GenTree::ChangeOperUnchecked(genTreeOps oper)
Expand Down
29 changes: 6 additions & 23 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12591,28 +12591,20 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
lclFld->SetLclOffs(lclFld->GetLclOffs() + static_cast<unsigned>(ival1));
lclFld->SetFieldSeq(GetFieldSeqStore()->Append(lclFld->GetFieldSeq(), fieldSeq));
}
else // we have a GT_LCL_VAR
else // We have a GT_LCL_VAR.
{
assert(temp->OperGet() == GT_LCL_VAR);
temp->ChangeOper(GT_LCL_FLD); // Note that this typically makes the gtFieldSeq "NotAField",
// unless there is a zero filed offset associated with 'temp'.
temp->ChangeOper(GT_LCL_FLD); // Note that this makes the gtFieldSeq "NotAField".
lclFld = temp->AsLclFld();
lclFld->SetLclOffs(static_cast<unsigned>(ival1));

if (lclFld->GetFieldSeq() == FieldSeqStore::NotAField())
if (fieldSeq != nullptr)
{
if (fieldSeq != nullptr)
{
// If it does represent a field, note that.
lclFld->SetFieldSeq(fieldSeq);
}
}
else
{
// Append 'fieldSeq' to the existing one
lclFld->SetFieldSeq(GetFieldSeqStore()->Append(lclFld->GetFieldSeq(), fieldSeq));
// If it does represent a field, note that.
lclFld->SetFieldSeq(fieldSeq);
}
}

temp->gtType = tree->gtType;
foldAndReturnTemp = true;
}
Expand Down Expand Up @@ -17806,15 +17798,6 @@ void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZ
fieldSeqRecorded = true;
break;

case GT_LCL_FLD:
{
GenTreeLclFld* lclFld = addr->AsLclFld();
fieldSeqUpdate = GetFieldSeqStore()->Append(lclFld->GetFieldSeq(), fieldSeqZero);
lclFld->SetFieldSeq(fieldSeqUpdate);
fieldSeqRecorded = true;
break;
}

case GT_ADDR:
if (addr->AsOp()->gtOp1->OperGet() == GT_LCL_FLD)
{
Expand Down
11 changes: 11 additions & 0 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8449,6 +8449,17 @@ void Compiler::fgValueNumberTree(GenTree* tree)
{
ValueNumPair lclVNPair = varDsc->GetPerSsaData(ssaNum)->m_vnPair;
tree->gtVNPair = vnStore->VNPairApplySelectors(lclVNPair, lclFld->GetFieldSeq(), indType);

// If we have byref field, we may have a zero-offset sequence to add.
FieldSeqNode* zeroOffsetFldSeq = nullptr;
if ((typ == TYP_BYREF) && GetZeroOffsetFieldMap()->Lookup(lclFld, &zeroOffsetFldSeq))
{
ValueNum addrExtended = vnStore->ExtendPtrVN(lclFld, zeroOffsetFldSeq);
if (addrExtended != ValueNumStore::NoVN)
{
lclFld->gtVNPair.SetBoth(addrExtended);
}
}
}
}
else
Expand Down