Skip to content

Commit 75ae3b5

Browse files
briansullbriansull
committed
Fixes for Zero Offset field sequence tracking
 - A GT_LCL_VAR may have a zeroOffset field - Add an assert to prevent building field sequences with duplicates - Fix fgMorphField when we have a zero offset field Improve fgAddFieldSeqForZeroOffset - Add JItDump info - Handle GT_LCL_FLD Changing the sign of an int constant also remove any field sequence information. Added method header comment for fgAddFieldSeqForZeroOffset Changed when we call fgAddFieldSeqForZeroOffset to be before the call to fgMorphSmpOp. Prefer calling fgAddFieldSeqForZeroOffset() to GetZeroOffsetFieldMap()->Set()
1 parent 6a7bf9c commit 75ae3b5

File tree

6 files changed

+193
-78
lines changed

6 files changed

+193
-78
lines changed

‎src/jit/compiler.hpp

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1451,9 +1451,24 @@ inline void GenTree::ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
14511451
switch (oper)
14521452
{
14531453
case GT_LCL_FLD:
1454+
{
1455+
// The original GT_LCL_VAR might be annotated with a zeroOffset field.
1456+
FieldSeqNode* zeroFieldSeq = nullptr;
1457+
Compiler* compiler = JitTls::GetCompiler();
1458+
bool isZeroOffset = compiler->GetZeroOffsetFieldMap()->Lookup(this, &zeroFieldSeq);
1459+
14541460
gtLclFld.gtLclOffs = 0;
14551461
gtLclFld.gtFieldSeq = FieldSeqStore::NotAField();
1462+
1463+
if (zeroFieldSeq != nullptr)
1464+
{
1465+
// Set the zeroFieldSeq in the GT_LCL_FLD node
1466+
gtLclFld.gtFieldSeq = zeroFieldSeq;
1467+
// and remove the annotation from the ZeroOffsetFieldMap
1468+
compiler->GetZeroOffsetFieldMap()->Remove(this);
1469+
}
14561470
break;
1471+
}
14571472
default:
14581473
break;
14591474
}

‎src/jit/gentree.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7415,7 +7415,7 @@ GenTree* Compiler::gtCloneExpr(
74157415
FieldSeqNode* fldSeq = nullptr;
74167416
if (GetZeroOffsetFieldMap()->Lookup(tree, &fldSeq))
74177417
{
7418-
GetZeroOffsetFieldMap()->Set(copy, fldSeq);
7418+
fgAddFieldSeqForZeroOffset(copy, fldSeq);
74197419
}
74207420
}
74217421

@@ -17628,6 +17628,9 @@ FieldSeqNode* FieldSeqStore::Append(FieldSeqNode* a, FieldSeqNode* b)
1762817628
}
1762917629
else
1763017630
{
17631+
// We should never add a duplicate FieldSeqNode
17632+
assert(a != b);
17633+
1763117634
FieldSeqNode* tmp = Append(a->m_next, b);
1763217635
FieldSeqNode fsn(a->m_fieldHnd, tmp);
1763317636
FieldSeqNode* res = nullptr;

‎src/jit/importer.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1330,7 +1330,8 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
13301330

13311331
assert(OFFSETOF__CORINFO_TypedReference__dataPtr == 0);
13321332
assert(destAddr->gtType == TYP_I_IMPL || destAddr->gtType == TYP_BYREF);
1333-
GetZeroOffsetFieldMap()->Set(destAddr, GetFieldSeqStore()->CreateSingleton(GetRefanyDataField()));
1333+
fgAddFieldSeqForZeroOffset(destAddr, GetFieldSeqStore()->CreateSingleton(GetRefanyDataField()));
1334+
13341335
GenTree* ptrSlot = gtNewOperNode(GT_IND, TYP_I_IMPL, destAddr);
13351336
GenTreeIntCon* typeFieldOffset = gtNewIconNode(OFFSETOF__CORINFO_TypedReference__type, TYP_I_IMPL);
13361337
typeFieldOffset->gtFieldSeq = GetFieldSeqStore()->CreateSingleton(GetRefanyTypeField());

‎src/jit/morph.cpp

Lines changed: 170 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -5975,7 +5975,7 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree)
59755975
if (cnsOff == nullptr) // It must have folded into a zero offset
59765976
{
59775977
// Record in the general zero-offset map.
5978-
GetZeroOffsetFieldMap()->Set(addr, fieldSeq);
5978+
fgAddFieldSeqForZeroOffset(addr, fieldSeq);
59795979
}
59805980
else
59815981
{
@@ -6398,14 +6398,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac)
63986398

63996399
addr = gtNewLclvNode(lclNum, objRefType); // Use "tmpLcl" to create "addr" node.
64006400
}
6401-
else if (fldOffset == 0)
6402-
{
6403-
// Generate the "addr" node.
6404-
addr = objRef;
6405-
FieldSeqNode* fieldSeq =
6406-
fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd);
6407-
GetZeroOffsetFieldMap()->Set(addr, fieldSeq);
6408-
}
64096401
else
64106402
{
64116403
addr = objRef;
@@ -6647,19 +6639,55 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac)
66476639
}
66486640
noway_assert(tree->gtOper == GT_IND);
66496641

6650-
// Pass down the current mac; if non null we are computing an address
6651-
GenTree* res = fgMorphSmpOp(tree, mac);
6652-
6653-
if (fldOffset == 0 && res->OperGet() == GT_IND)
6642+
if (fldOffset == 0)
66546643
{
6655-
GenTree* addr = res->gtOp.gtOp1;
6644+
GenTree* addr = tree->gtOp.gtOp1;
6645+
6646+
// 'addr' may be a GT_COMMA. Skip over any comma nodes
6647+
addr = addr->gtEffectiveVal();
6648+
6649+
#ifdef DEBUG
6650+
if (verbose)
6651+
{
6652+
printf("\nBefore calling fgAddFieldSeqForZeroOffset:\n");
6653+
gtDispTree(tree);
6654+
}
6655+
#endif
6656+
6657+
// We expect 'addr' to be an address at this point.
6658+
// But there are also cases where we can see a GT_LCL_FLD or a GT_LCL_VAR:
6659+
//
6660+
// [001076] ----G------- /--* FIELD long m_constArray
6661+
// [001072] ----G------- | \--* ADDR byref
6662+
// [001073] ----G------- | \--* FIELD struct blob
6663+
// [001074] ------------ | \--* ADDR byref
6664+
// [001075] ------------ | \--* LCL_VAR struct V18 loc11
6665+
//
6666+
//
6667+
assert((addr->TypeGet() == TYP_BYREF) || (addr->TypeGet() == TYP_I_IMPL) || (addr->OperGet() == GT_LCL_FLD) ||
6668+
(addr->OperGet() == GT_LCL_VAR));
6669+
66566670
// Since we don't make a constant zero to attach the field sequence to, associate it with the "addr" node.
66576671
FieldSeqNode* fieldSeq =
66586672
fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd);
66596673
fgAddFieldSeqForZeroOffset(addr, fieldSeq);
66606674
}
66616675

6662-
return res;
6676+
// Pass down the current mac; if non null we are computing an address
6677+
GenTree* result = fgMorphSmpOp(tree, mac);
6678+
6679+
#ifdef DEBUG
6680+
if (fldOffset == 0)
6681+
{
6682+
if (verbose)
6683+
{
6684+
printf("\nAfter calling fgMorphSmpOp (zero fldOffset case):\n");
6685+
gtDispTree(result);
6686+
}
6687+
}
6688+
#endif
6689+
6690+
return result;
66636691
}
66646692

66656693
//------------------------------------------------------------------------------
@@ -12897,7 +12925,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1289712925
/* Negate the constant and change the node to be "+" */
1289812926

1289912927
op2->gtIntConCommon.SetIconValue(-op2->gtIntConCommon.IconValue());
12900-
oper = GT_ADD;
12928+
op2->gtIntCon.gtFieldSeq = FieldSeqStore::NotAField();
12929+
oper = GT_ADD;
1290112930
tree->ChangeOper(oper);
1290212931
goto CM_ADD_OP;
1290312932
}
@@ -13472,13 +13501,25 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1347213501
temp->AsLclFld()->gtFieldSeq =
1347313502
GetFieldSeqStore()->Append(temp->AsLclFld()->gtFieldSeq, fieldSeq);
1347413503
}
13475-
else
13504+
else // we have a GT_LCL_VAR
1347613505
{
13477-
temp->ChangeOper(GT_LCL_FLD); // Note that this makes the gtFieldSeq "NotAField"...
13506+
assert(temp->OperGet() == GT_LCL_VAR);
13507+
temp->ChangeOper(GT_LCL_FLD); // Note that this typically makes the gtFieldSeq "NotAField"...
1347813508
temp->AsLclFld()->gtLclOffs = (unsigned short)ival1;
13479-
if (fieldSeq != nullptr)
13480-
{ // If it does represent a field, note that.
13481-
temp->AsLclFld()->gtFieldSeq = fieldSeq;
13509+
13510+
if (temp->AsLclFld()->gtFieldSeq == FieldSeqStore::NotAField())
13511+
{
13512+
if (fieldSeq != nullptr)
13513+
{
13514+
// If it does represent a field, note that.
13515+
temp->AsLclFld()->gtFieldSeq = fieldSeq;
13516+
}
13517+
}
13518+
else
13519+
{
13520+
// Append 'fieldSeq' to the exsisting one
13521+
temp->AsLclFld()->gtFieldSeq =
13522+
GetFieldSeqStore()->Append(temp->AsLclFld()->gtFieldSeq, fieldSeq);
1348213523
}
1348313524
}
1348413525
temp->gtType = tree->gtType;
@@ -13689,7 +13730,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
1368913730
zeroFieldSeq = GetFieldSeqStore()->Append(existingZeroOffsetFldSeq, zeroFieldSeq);
1369013731
}
1369113732
// Transfer the annotation to the new GT_ADDR node.
13692-
GetZeroOffsetFieldMap()->Set(op1, zeroFieldSeq, NodeToFieldSeqMap::Overwrite);
13733+
fgAddFieldSeqForZeroOffset(op1, zeroFieldSeq);
1369313734
}
1369413735
commaNode->gtOp.gtOp2 = op1;
1369513736
// Originally, I gave all the comma nodes type "byref". But the ADDR(IND(x)) == x transform
@@ -18703,66 +18744,132 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
1870318744
}
1870418745
};
1870518746

18706-
void Compiler::fgAddFieldSeqForZeroOffset(GenTree* op1, FieldSeqNode* fieldSeq)
18747+
//------------------------------------------------------------------------
18748+
// fgAddFieldSeqForZeroOffset:
18749+
// Associate a fieldSeq (with a zero offset) with the GenTree node 'addr'
18750+
//
18751+
// Arguments:
18752+
// addr - A GenTree node
18753+
// fieldSeqZero - a fieldSeq (with a zero offset)
18754+
//
18755+
// Notes:
18756+
// Some GenTree nodes have internal fields that record the field sequence.
18757+
// If we have one of these nodes: GT_CNS_INT, GT_LCL_FLD
18758+
// we can append the field sequence using the gtFieldSeq
18759+
// If we have a GT_ADD of a GT_CNS_INT we can use the
18760+
// fieldSeq from child node.
18761+
// Otherwise we record 'fieldSeqZero' in the GenTree node using
18762+
// a Map: GetFieldSeqStore()
18763+
// When doing so we take care to preserve any existing zero field sequence
18764+
//
18765+
void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZero)
1870718766
{
18708-
assert(op1->TypeGet() == TYP_BYREF || op1->TypeGet() == TYP_I_IMPL || op1->TypeGet() == TYP_REF);
18767+
// We expect 'addr' to be an address at this point.
18768+
// But there are also cases where we can see a GT_LCL_FLD or a GT_LCL_VAR:
18769+
//
18770+
// [001076] ----G------- /--* FIELD long m_constArray
18771+
// [001072] ----G------- | \--* ADDR byref
18772+
// [001073] ----G------- | \--* FIELD struct blob
18773+
// [001074] ------------ | \--* ADDR byref
18774+
// [001075] ------------ | \--* LCL_VAR struct V18 loc11
18775+
//
18776+
//
18777+
assert(addr->TypeGet() == TYP_BYREF || addr->TypeGet() == TYP_I_IMPL || addr->OperGet() == GT_LCL_FLD ||
18778+
addr->OperGet() == GT_LCL_VAR);
18779+
18780+
FieldSeqNode* fieldSeqUpdate = fieldSeqZero;
18781+
GenTree* fieldSeqNode = addr;
18782+
bool fieldSeqRecorded = false;
18783+
bool isMapAnnotation = false;
18784+
18785+
#ifdef DEBUG
18786+
if (verbose)
18787+
{
18788+
printf("\nfgAddFieldSeqForZeroOffset for");
18789+
gtDispFieldSeq(fieldSeqZero);
18790+
18791+
printf("\naddr (Before)\n");
18792+
gtDispNode(addr, nullptr, nullptr, false);
18793+
gtDispCommonEndLine(addr);
18794+
}
18795+
#endif // DEBUG
1870918796

18710-
switch (op1->OperGet())
18797+
switch (addr->OperGet())
1871118798
{
18799+
case GT_CNS_INT:
18800+
fieldSeqUpdate = GetFieldSeqStore()->Append(addr->gtIntCon.gtFieldSeq, fieldSeqZero);
18801+
addr->gtIntCon.gtFieldSeq = fieldSeqUpdate;
18802+
fieldSeqRecorded = true;
18803+
break;
18804+
18805+
case GT_LCL_FLD:
18806+
{
18807+
GenTreeLclFld* lclFld = addr->AsLclFld();
18808+
fieldSeqUpdate = GetFieldSeqStore()->Append(lclFld->gtFieldSeq, fieldSeqZero);
18809+
lclFld->gtFieldSeq = fieldSeqUpdate;
18810+
fieldSeqRecorded = true;
18811+
break;
18812+
}
18813+
1871218814
case GT_ADDR:
18713-
if (op1->gtOp.gtOp1->OperGet() == GT_LCL_FLD)
18815+
if (addr->gtOp.gtOp1->OperGet() == GT_LCL_FLD)
1871418816
{
18715-
GenTreeLclFld* lclFld = op1->gtOp.gtOp1->AsLclFld();
18716-
lclFld->gtFieldSeq = GetFieldSeqStore()->Append(lclFld->gtFieldSeq, fieldSeq);
18817+
fieldSeqNode = addr->gtOp.gtOp1;
18818+
18819+
GenTreeLclFld* lclFld = addr->gtOp.gtOp1->AsLclFld();
18820+
fieldSeqUpdate = GetFieldSeqStore()->Append(lclFld->gtFieldSeq, fieldSeqZero);
18821+
lclFld->gtFieldSeq = fieldSeqUpdate;
18822+
fieldSeqRecorded = true;
1871718823
}
1871818824
break;
1871918825

1872018826
case GT_ADD:
18721-
if (op1->gtOp.gtOp1->OperGet() == GT_CNS_INT)
18827+
if (addr->gtOp.gtOp1->OperGet() == GT_CNS_INT)
1872218828
{
18723-
FieldSeqNode* op1Fs = op1->gtOp.gtOp1->gtIntCon.gtFieldSeq;
18724-
if (op1Fs != nullptr)
18725-
{
18726-
op1Fs = GetFieldSeqStore()->Append(op1Fs, fieldSeq);
18727-
op1->gtOp.gtOp1->gtIntCon.gtFieldSeq = op1Fs;
18728-
}
18829+
fieldSeqNode = addr->gtOp.gtOp1;
18830+
18831+
fieldSeqUpdate = GetFieldSeqStore()->Append(addr->gtOp.gtOp1->gtIntCon.gtFieldSeq, fieldSeqZero);
18832+
addr->gtOp.gtOp1->gtIntCon.gtFieldSeq = fieldSeqUpdate;
18833+
fieldSeqRecorded = true;
1872918834
}
18730-
else if (op1->gtOp.gtOp2->OperGet() == GT_CNS_INT)
18835+
else if (addr->gtOp.gtOp2->OperGet() == GT_CNS_INT)
1873118836
{
18732-
FieldSeqNode* op2Fs = op1->gtOp.gtOp2->gtIntCon.gtFieldSeq;
18733-
if (op2Fs != nullptr)
18734-
{
18735-
op2Fs = GetFieldSeqStore()->Append(op2Fs, fieldSeq);
18736-
op1->gtOp.gtOp2->gtIntCon.gtFieldSeq = op2Fs;
18737-
}
18837+
fieldSeqNode = addr->gtOp.gtOp2;
18838+
18839+
fieldSeqUpdate = GetFieldSeqStore()->Append(addr->gtOp.gtOp2->gtIntCon.gtFieldSeq, fieldSeqZero);
18840+
addr->gtOp.gtOp2->gtIntCon.gtFieldSeq = fieldSeqUpdate;
18841+
fieldSeqRecorded = true;
1873818842
}
1873918843
break;
1874018844

18741-
case GT_CNS_INT:
18845+
default:
18846+
break;
18847+
}
18848+
18849+
if (fieldSeqRecorded == false)
18850+
{
18851+
// Record in the general zero-offset map.
18852+
18853+
// The "addr" node might already be annotated with a zero-offset field sequence.
18854+
FieldSeqNode* existingFieldSeq = nullptr;
18855+
if (GetZeroOffsetFieldMap()->Lookup(addr, &existingFieldSeq))
1874218856
{
18743-
FieldSeqNode* op1Fs = op1->gtIntCon.gtFieldSeq;
18744-
if (op1Fs != nullptr)
18745-
{
18746-
op1Fs = GetFieldSeqStore()->Append(op1Fs, fieldSeq);
18747-
op1->gtIntCon.gtFieldSeq = op1Fs;
18748-
}
18857+
// Append the zero field sequences
18858+
fieldSeqUpdate = GetFieldSeqStore()->Append(existingFieldSeq, fieldSeqZero);
1874918859
}
18750-
break;
18751-
18752-
default:
18753-
// Record in the general zero-offset map.
18860+
// Overwrite the field sequence annotation for op1
18861+
GetZeroOffsetFieldMap()->Set(addr, fieldSeqUpdate, NodeToFieldSeqMap::Overwrite);
18862+
fieldSeqRecorded = true;
18863+
}
1875418864

18755-
// The "op1" node might already be annotated with a zero-offset field sequence.
18756-
FieldSeqNode* existingZeroOffsetFldSeq = nullptr;
18757-
if (GetZeroOffsetFieldMap()->Lookup(op1, &existingZeroOffsetFldSeq))
18758-
{
18759-
// Append the zero field sequences
18760-
fieldSeq = GetFieldSeqStore()->Append(existingZeroOffsetFldSeq, fieldSeq);
18761-
}
18762-
// Set the new field sequence annotation for op1
18763-
GetZeroOffsetFieldMap()->Set(op1, fieldSeq, NodeToFieldSeqMap::Overwrite);
18764-
break;
18865+
#ifdef DEBUG
18866+
if (verbose)
18867+
{
18868+
printf(" (After)\n");
18869+
gtDispNode(fieldSeqNode, nullptr, nullptr, false);
18870+
gtDispCommonEndLine(fieldSeqNode);
1876518871
}
18872+
#endif // DEBUG
1876618873
}
1876718874

1876818875
//------------------------------------------------------------------------

‎src/jit/optcse.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2351,7 +2351,7 @@ class CSE_Heuristic
23512351
// If it has a zero-offset field seq, copy annotation to the ref
23522352
if (hasZeroMapAnnotation)
23532353
{
2354-
m_pCompiler->GetZeroOffsetFieldMap()->Set(ref, fldSeq);
2354+
m_pCompiler->fgAddFieldSeqForZeroOffset(ref, fldSeq);
23552355
}
23562356

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

23982398
assert(m_pCompiler->fgRemoveRestOfBlock == false);

‎src/jit/valuenum.cpp

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7510,17 +7510,6 @@ void Compiler::fgValueNumberTree(GenTree* tree)
75107510
ValueNum arrVN = funcApp.m_args[1];
75117511
ValueNum inxVN = funcApp.m_args[2];
75127512
FieldSeqNode* fldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[3]);
7513-
7514-
if (arg->gtOper != GT_LCL_VAR)
7515-
{
7516-
// Does the child of the GT_IND 'arg' have an associated zero-offset field sequence?
7517-
FieldSeqNode* addrFieldSeq = nullptr;
7518-
if (GetZeroOffsetFieldMap()->Lookup(arg, &addrFieldSeq))
7519-
{
7520-
fldSeq = GetFieldSeqStore()->Append(addrFieldSeq, fldSeq);
7521-
}
7522-
}
7523-
75247513
#ifdef DEBUG
75257514
if (verbose)
75267515
{

0 commit comments

Comments
 (0)