Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fixes for tracking struct field sequences #23932

Merged
merged 3 commits into from
Apr 23, 2019
Merged

Fixes for tracking struct field sequences #23932

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
75ae3b5
Fixes for Zero Offset field sequence tracking
briansull Apr 16, 2019
22028dc
Code review feedback
briansull Apr 20, 2019
dbf2f74
Added additional comment explaining ChnageOper(GT_LCL_FLD) and NotAField
briansull Apr 23, 2019
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
15 changes: 15 additions & 0 deletions src/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1451,9 +1451,24 @@ inline void GenTree::ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
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);

gtLclFld.gtLclOffs = 0;
gtLclFld.gtFieldSeq = FieldSeqStore::NotAField();

if (zeroFieldSeq != nullptr)
{
// Set the zeroFieldSeq in the GT_LCL_FLD node
gtLclFld.gtFieldSeq = zeroFieldSeq;
// and remove the annotation from the ZeroOffsetFieldMap
compiler->GetZeroOffsetFieldMap()->Remove(this);
}
break;
}
default:
break;
}
Expand Down
5 changes: 4 additions & 1 deletion src/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7415,7 +7415,7 @@ GenTree* Compiler::gtCloneExpr(
FieldSeqNode* fldSeq = nullptr;
if (GetZeroOffsetFieldMap()->Lookup(tree, &fldSeq))
{
GetZeroOffsetFieldMap()->Set(copy, fldSeq);
fgAddFieldSeqForZeroOffset(copy, fldSeq);
}
}

Expand Down Expand Up @@ -17628,6 +17628,9 @@ FieldSeqNode* FieldSeqStore::Append(FieldSeqNode* a, FieldSeqNode* b)
}
else
{
// We should never add a duplicate FieldSeqNode
assert(a != b);

FieldSeqNode* tmp = Append(a->m_next, b);
FieldSeqNode fsn(a->m_fieldHnd, tmp);
FieldSeqNode* res = nullptr;
Expand Down
3 changes: 2 additions & 1 deletion src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,7 +1330,8 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,

assert(OFFSETOF__CORINFO_TypedReference__dataPtr == 0);
assert(destAddr->gtType == TYP_I_IMPL || destAddr->gtType == TYP_BYREF);
GetZeroOffsetFieldMap()->Set(destAddr, GetFieldSeqStore()->CreateSingleton(GetRefanyDataField()));
fgAddFieldSeqForZeroOffset(destAddr, GetFieldSeqStore()->CreateSingleton(GetRefanyDataField()));

GenTree* ptrSlot = gtNewOperNode(GT_IND, TYP_I_IMPL, destAddr);
GenTreeIntCon* typeFieldOffset = gtNewIconNode(OFFSETOF__CORINFO_TypedReference__type, TYP_I_IMPL);
typeFieldOffset->gtFieldSeq = GetFieldSeqStore()->CreateSingleton(GetRefanyTypeField());
Expand Down
221 changes: 154 additions & 67 deletions src/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5975,7 +5975,7 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree)
if (cnsOff == nullptr) // It must have folded into a zero offset
{
// Record in the general zero-offset map.
GetZeroOffsetFieldMap()->Set(addr, fieldSeq);
fgAddFieldSeqForZeroOffset(addr, fieldSeq);
}
else
{
Expand Down Expand Up @@ -6398,14 +6398,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac)

addr = gtNewLclvNode(lclNum, objRefType); // Use "tmpLcl" to create "addr" node.
}
else if (fldOffset == 0)
{
// Generate the "addr" node.
addr = objRef;
FieldSeqNode* fieldSeq =
fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd);
GetZeroOffsetFieldMap()->Set(addr, fieldSeq);
}
else
{
addr = objRef;
Expand Down Expand Up @@ -6647,19 +6639,42 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac)
}
noway_assert(tree->gtOper == GT_IND);

// Pass down the current mac; if non null we are computing an address
GenTree* res = fgMorphSmpOp(tree, mac);

if (fldOffset == 0 && res->OperGet() == GT_IND)
if (fldOffset == 0)
{
GenTree* addr = res->gtOp.gtOp1;
GenTree* addr = tree->gtOp.gtOp1;

// 'addr' may be a GT_COMMA. Skip over any comma nodes
addr = addr->gtEffectiveVal();

#ifdef DEBUG
if (verbose)
{
printf("\nBefore calling fgAddFieldSeqForZeroOffset:\n");
Copy link

Choose a reason for hiding this comment

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

This seems redundant because fgAddFieldSeqForZeroOffset also does a "before" dump.

Copy link
Author

Choose a reason for hiding this comment

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

Actually This will print the whole tree (and it occurs after the call to fgMorphSmpOp)
It is helpful to see the final version of the tree to verify that the field sequences are correct and non of them went missing during the call to fgMorphSmpOp

Copy link
Author

@briansull briansull Apr 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If you want, I could change this to always print the final result of Compiler::fgMorphField.
That might be better, since the morphing of a GT_FIELD produces a new more complex tree.

Copy link

Choose a reason for hiding this comment

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

That makes sense to me

Copy link
Author

Choose a reason for hiding this comment

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

Carol after my last change the assert can now be changed to simply check that the type is BYREF or I_IMPL.

// We expect 'addr' to be an address at this point.
assert(addr->TypeGet() == TYP_BYREF || addr->TypeGet() == TYP_I_IMPL);

Copy link

Choose a reason for hiding this comment

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

That's great!

gtDispTree(tree);
}
#endif

// We expect 'addr' to be an address at this point.
assert(addr->TypeGet() == TYP_BYREF || addr->TypeGet() == TYP_I_IMPL);

briansull marked this conversation as resolved.
Show resolved Hide resolved
// Since we don't make a constant zero to attach the field sequence to, associate it with the "addr" node.
FieldSeqNode* fieldSeq =
fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd);
fgAddFieldSeqForZeroOffset(addr, fieldSeq);
}

return res;
// Pass down the current mac; if non null we are computing an address
GenTree* result = fgMorphSmpOp(tree, mac);

#ifdef DEBUG
if (verbose)
{
printf("\nFinal value of Compiler::fgMorphField after calling fgMorphSmpOp:\n");
gtDispTree(result);
}
#endif

return result;
}

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -12897,7 +12912,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
/* Negate the constant and change the node to be "+" */

op2->gtIntConCommon.SetIconValue(-op2->gtIntConCommon.IconValue());
oper = GT_ADD;
op2->gtIntCon.gtFieldSeq = FieldSeqStore::NotAField();
oper = GT_ADD;
tree->ChangeOper(oper);
goto CM_ADD_OP;
}
Expand Down Expand Up @@ -13462,23 +13478,38 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
// lclVar and must not extend beyond the end of the lclVar.
if ((ival1 >= 0) && ((ival1 + genTypeSize(typ)) <= varSize))
{
GenTreeLclFld* lclFld;

// We will turn a GT_LCL_VAR into a GT_LCL_FLD with an gtLclOffs of 'ival'
// or if we already have a GT_LCL_FLD we will adjust the gtLclOffs by adding 'ival'
// Then we change the type of the GT_LCL_FLD to match the orginal GT_IND type.
//
if (temp->OperGet() == GT_LCL_FLD)
{
temp->AsLclFld()->gtLclOffs += (unsigned short)ival1;
temp->AsLclFld()->gtFieldSeq =
GetFieldSeqStore()->Append(temp->AsLclFld()->gtFieldSeq, fieldSeq);
lclFld = temp->AsLclFld();
lclFld->gtLclOffs += (unsigned short)ival1;
lclFld->gtFieldSeq = GetFieldSeqStore()->Append(lclFld->gtFieldSeq, fieldSeq);
}
else
else // we have a GT_LCL_VAR
{
temp->ChangeOper(GT_LCL_FLD); // Note that this makes the gtFieldSeq "NotAField"...
temp->AsLclFld()->gtLclOffs = (unsigned short)ival1;
if (fieldSeq != nullptr)
{ // If it does represent a field, note that.
temp->AsLclFld()->gtFieldSeq = fieldSeq;
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'.
Copy link

Choose a reason for hiding this comment

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

The formatting here is weird, but it's not a big deal I guess.

lclFld = temp->AsLclFld();
lclFld->gtLclOffs = (unsigned short)ival1;

if (lclFld->gtFieldSeq == FieldSeqStore::NotAField())
{
if (fieldSeq != nullptr)
{
// If it does represent a field, note that.
lclFld->gtFieldSeq = fieldSeq;
}
}
else
{
// Append 'fieldSeq' to the existing one
lclFld->gtFieldSeq = GetFieldSeqStore()->Append(lclFld->gtFieldSeq, fieldSeq);
}
}
temp->gtType = tree->gtType;
Expand Down Expand Up @@ -13689,7 +13720,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
zeroFieldSeq = GetFieldSeqStore()->Append(existingZeroOffsetFldSeq, zeroFieldSeq);
}
// Transfer the annotation to the new GT_ADDR node.
GetZeroOffsetFieldMap()->Set(op1, zeroFieldSeq, NodeToFieldSeqMap::Overwrite);
fgAddFieldSeqForZeroOffset(op1, zeroFieldSeq);
}
commaNode->gtOp.gtOp2 = op1;
// Originally, I gave all the comma nodes type "byref". But the ADDR(IND(x)) == x transform
Expand Down Expand Up @@ -18703,66 +18734,122 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
}
};

void Compiler::fgAddFieldSeqForZeroOffset(GenTree* op1, FieldSeqNode* fieldSeq)
//------------------------------------------------------------------------
// fgAddFieldSeqForZeroOffset:
// Associate a fieldSeq (with a zero offset) with the GenTree node 'addr'
//
// Arguments:
// addr - A GenTree node
// fieldSeqZero - a fieldSeq (with a zero offset)
//
// Notes:
// Some GenTree nodes have internal fields that record the field sequence.
// If we have one of these nodes: GT_CNS_INT, GT_LCL_FLD
// we can append the field sequence using the gtFieldSeq
// If we have a GT_ADD of a GT_CNS_INT we can use the
// fieldSeq from child node.
// Otherwise we record 'fieldSeqZero' in the GenTree node using
// a Map: GetFieldSeqStore()
// When doing so we take care to preserve any existing zero field sequence
//
void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZero)
briansull marked this conversation as resolved.
Show resolved Hide resolved
{
assert(op1->TypeGet() == TYP_BYREF || op1->TypeGet() == TYP_I_IMPL || op1->TypeGet() == TYP_REF);
// We expect 'addr' to be an address at this point.
assert(addr->TypeGet() == TYP_BYREF || addr->TypeGet() == TYP_I_IMPL);

switch (op1->OperGet())
FieldSeqNode* fieldSeqUpdate = fieldSeqZero;
briansull marked this conversation as resolved.
Show resolved Hide resolved
GenTree* fieldSeqNode = addr;
bool fieldSeqRecorded = false;
bool isMapAnnotation = false;

#ifdef DEBUG
if (verbose)
{
printf("\nfgAddFieldSeqForZeroOffset for");
gtDispFieldSeq(fieldSeqZero);

printf("\naddr (Before)\n");
gtDispNode(addr, nullptr, nullptr, false);
gtDispCommonEndLine(addr);
}
#endif // DEBUG

switch (addr->OperGet())
{
case GT_CNS_INT:
fieldSeqUpdate = GetFieldSeqStore()->Append(addr->gtIntCon.gtFieldSeq, fieldSeqZero);
addr->gtIntCon.gtFieldSeq = fieldSeqUpdate;
fieldSeqRecorded = true;
break;

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

case GT_ADDR:
if (op1->gtOp.gtOp1->OperGet() == GT_LCL_FLD)
if (addr->gtOp.gtOp1->OperGet() == GT_LCL_FLD)
{
GenTreeLclFld* lclFld = op1->gtOp.gtOp1->AsLclFld();
lclFld->gtFieldSeq = GetFieldSeqStore()->Append(lclFld->gtFieldSeq, fieldSeq);
fieldSeqNode = addr->gtOp.gtOp1;

GenTreeLclFld* lclFld = addr->gtOp.gtOp1->AsLclFld();
fieldSeqUpdate = GetFieldSeqStore()->Append(lclFld->gtFieldSeq, fieldSeqZero);
lclFld->gtFieldSeq = fieldSeqUpdate;
fieldSeqRecorded = true;
}
break;

case GT_ADD:
if (op1->gtOp.gtOp1->OperGet() == GT_CNS_INT)
if (addr->gtOp.gtOp1->OperGet() == GT_CNS_INT)
{
FieldSeqNode* op1Fs = op1->gtOp.gtOp1->gtIntCon.gtFieldSeq;
if (op1Fs != nullptr)
{
op1Fs = GetFieldSeqStore()->Append(op1Fs, fieldSeq);
op1->gtOp.gtOp1->gtIntCon.gtFieldSeq = op1Fs;
}
fieldSeqNode = addr->gtOp.gtOp1;

fieldSeqUpdate = GetFieldSeqStore()->Append(addr->gtOp.gtOp1->gtIntCon.gtFieldSeq, fieldSeqZero);
addr->gtOp.gtOp1->gtIntCon.gtFieldSeq = fieldSeqUpdate;
fieldSeqRecorded = true;
}
else if (op1->gtOp.gtOp2->OperGet() == GT_CNS_INT)
else if (addr->gtOp.gtOp2->OperGet() == GT_CNS_INT)
{
FieldSeqNode* op2Fs = op1->gtOp.gtOp2->gtIntCon.gtFieldSeq;
if (op2Fs != nullptr)
{
op2Fs = GetFieldSeqStore()->Append(op2Fs, fieldSeq);
op1->gtOp.gtOp2->gtIntCon.gtFieldSeq = op2Fs;
}
fieldSeqNode = addr->gtOp.gtOp2;

fieldSeqUpdate = GetFieldSeqStore()->Append(addr->gtOp.gtOp2->gtIntCon.gtFieldSeq, fieldSeqZero);
addr->gtOp.gtOp2->gtIntCon.gtFieldSeq = fieldSeqUpdate;
fieldSeqRecorded = true;
}
break;

case GT_CNS_INT:
default:
break;
}

if (fieldSeqRecorded == false)
{
// Record in the general zero-offset map.

// The "addr" node might already be annotated with a zero-offset field sequence.
FieldSeqNode* existingFieldSeq = nullptr;
if (GetZeroOffsetFieldMap()->Lookup(addr, &existingFieldSeq))
{
FieldSeqNode* op1Fs = op1->gtIntCon.gtFieldSeq;
if (op1Fs != nullptr)
{
op1Fs = GetFieldSeqStore()->Append(op1Fs, fieldSeq);
op1->gtIntCon.gtFieldSeq = op1Fs;
}
// Append the zero field sequences
fieldSeqUpdate = GetFieldSeqStore()->Append(existingFieldSeq, fieldSeqZero);
}
break;

default:
// Record in the general zero-offset map.
// Overwrite the field sequence annotation for op1
GetZeroOffsetFieldMap()->Set(addr, fieldSeqUpdate, NodeToFieldSeqMap::Overwrite);
fieldSeqRecorded = true;
}

// The "op1" node might already be annotated with a zero-offset field sequence.
FieldSeqNode* existingZeroOffsetFldSeq = nullptr;
if (GetZeroOffsetFieldMap()->Lookup(op1, &existingZeroOffsetFldSeq))
{
// Append the zero field sequences
fieldSeq = GetFieldSeqStore()->Append(existingZeroOffsetFldSeq, fieldSeq);
}
// Set the new field sequence annotation for op1
GetZeroOffsetFieldMap()->Set(op1, fieldSeq, NodeToFieldSeqMap::Overwrite);
break;
#ifdef DEBUG
if (verbose)
{
printf(" (After)\n");
gtDispNode(fieldSeqNode, nullptr, nullptr, false);
gtDispCommonEndLine(fieldSeqNode);
}
#endif // DEBUG
}

//------------------------------------------------------------------------
Expand Down
4 changes: 2 additions & 2 deletions src/jit/optcse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2351,7 +2351,7 @@ class CSE_Heuristic
// If it has a zero-offset field seq, copy annotation to the ref
if (hasZeroMapAnnotation)
{
m_pCompiler->GetZeroOffsetFieldMap()->Set(ref, fldSeq);
m_pCompiler->fgAddFieldSeqForZeroOffset(ref, fldSeq);
}

/* Create a comma node for the CSE assignment */
Expand Down Expand Up @@ -2392,7 +2392,7 @@ class CSE_Heuristic
// If it has a zero-offset field seq, copy annotation.
if (hasZeroMapAnnotation)
{
m_pCompiler->GetZeroOffsetFieldMap()->Set(cse, fldSeq);
m_pCompiler->fgAddFieldSeqForZeroOffset(cse, fldSeq);
}

assert(m_pCompiler->fgRemoveRestOfBlock == false);
Expand Down
11 changes: 0 additions & 11 deletions src/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7510,17 +7510,6 @@ void Compiler::fgValueNumberTree(GenTree* tree)
ValueNum arrVN = funcApp.m_args[1];
ValueNum inxVN = funcApp.m_args[2];
FieldSeqNode* fldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[3]);

if (arg->gtOper != GT_LCL_VAR)
{
// Does the child of the GT_IND 'arg' have an associated zero-offset field sequence?
FieldSeqNode* addrFieldSeq = nullptr;
if (GetZeroOffsetFieldMap()->Lookup(arg, &addrFieldSeq))
{
fldSeq = GetFieldSeqStore()->Append(addrFieldSeq, fldSeq);
}
}

#ifdef DEBUG
if (verbose)
{
Expand Down