Skip to content

Forbid creating layout-less TYP_STRUCT LCL_FLD nodes #70170

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
merged 5 commits into from
Jun 12, 2022
Merged

Forbid creating layout-less TYP_STRUCT LCL_FLD nodes #70170

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
d27b226
Fold ADD(ADDR(LCL_VAR), CONST) => ADDR(LCL_FLD)
SingleAccretion May 28, 2022
b58ba87
Add an assert for layout-less LCL_FLD
SingleAccretion Jun 7, 2022
4796d11
Query the handle in "gtGetStructHandleIfPresent"
SingleAccretion Jun 2, 2022
13fd37a
Add layout checks to "Compare" and "gtHashValue"
SingleAccretion Jun 2, 2022
1e70d81
Bring back the CSE check
SingleAccretion Jun 12, 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
10 changes: 8 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2398,7 +2398,8 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK)

case GT_LCL_FLD:
if ((op1->AsLclFld()->GetLclNum() != op2->AsLclFld()->GetLclNum()) ||
(op1->AsLclFld()->GetLclOffs() != op2->AsLclFld()->GetLclOffs()))
(op1->AsLclFld()->GetLclOffs() != op2->AsLclFld()->GetLclOffs()) ||
(op1->AsLclFld()->GetLayout() != op2->AsLclFld()->GetLayout()))
{
break;
}
Expand Down Expand Up @@ -2800,6 +2801,7 @@ unsigned Compiler::gtHashValue(GenTree* tree)
break;
case GT_LCL_FLD:
hash = genTreeHashAdd(hash, tree->AsLclFld()->GetLclNum());
hash = genTreeHashAdd(hash, tree->AsLclFld()->GetLayout());
add = tree->AsLclFld()->GetLclOffs();
break;

Expand Down Expand Up @@ -17230,7 +17232,11 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
}
#endif
}
else
#endif
{
structHnd = tree->AsLclFld()->GetLayout()->GetClassHandle();
}
break;
case GT_LCL_VAR:
{
Expand Down Expand Up @@ -23181,7 +23187,7 @@ unsigned GenTreeHWIntrinsic::GetResultOpNumForFMA(GenTree* use, GenTree* op1, Ge

unsigned GenTreeLclFld::GetSize() const
{
return genTypeSize(TypeGet());
return TypeIs(TYP_STRUCT) ? GetLayout()->GetSize() : genTypeSize(TypeGet());
}

#ifdef TARGET_ARM
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3691,9 +3691,10 @@ struct GenTreeLclFld : public GenTreeLclVarCommon

public:
GenTreeLclFld(genTreeOps oper, var_types type, unsigned lclNum, unsigned lclOffs, ClassLayout* layout = nullptr)
: GenTreeLclVarCommon(oper, type, lclNum), m_lclOffs(static_cast<uint16_t>(lclOffs)), m_layout(layout)
: GenTreeLclVarCommon(oper, type, lclNum), m_lclOffs(static_cast<uint16_t>(lclOffs))
{
assert(lclOffs <= UINT16_MAX);
SetLayout(layout);
}

uint16_t GetLclOffs() const
Expand All @@ -3709,6 +3710,7 @@ struct GenTreeLclFld : public GenTreeLclVarCommon

ClassLayout* GetLayout() const
{
assert(!TypeIs(TYP_STRUCT) || (m_layout != nullptr));
return m_layout;
}

Expand Down
151 changes: 48 additions & 103 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11306,7 +11306,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
*/

GenTree* temp;
size_t ival1;
GenTree* lclVarTree;
GenTree* effectiveOp1;
FieldSeqNode* fieldSeq = nullptr;
Expand Down Expand Up @@ -11683,15 +11682,14 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)

bool foldAndReturnTemp = false;
temp = nullptr;
ival1 = 0;

// Don't remove a volatile GT_IND, even if the address points to a local variable.
if ((tree->gtFlags & GTF_IND_VOLATILE) == 0)
//
if (!tree->AsIndir()->IsVolatile())
{
/* Try to Fold *(&X) into X */
if (op1->gtOper == GT_ADDR)
{
// Can not remove a GT_ADDR if it is currently a CSE candidate.
if (gtIsActiveCSE_Candidate(op1))
{
break;
Expand Down Expand Up @@ -11770,20 +11768,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
}
else
{
// Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e.
// nullptr)
assert(fieldSeq == nullptr);
bool b = GetZeroOffsetFieldMap()->Lookup(op1, &fieldSeq);
assert(b || fieldSeq == nullptr);

if ((fieldSeq != nullptr) && (temp->OperGet() == GT_LCL_FLD))
{
temp->gtType = typ;
foldAndReturnTemp = true;
}
}
// Otherwise will will fold this into a GT_LCL_FLD below
// where we check (temp != nullptr)
}
Expand All @@ -11810,98 +11794,34 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
}
}
#endif // TARGET_ARM

/* Try to change *(&lcl + cns) into lcl[cns] to prevent materialization of &lcl */

if (op1->AsOp()->gtOp1->OperGet() == GT_ADDR && op1->AsOp()->gtOp2->OperGet() == GT_CNS_INT &&
opts.OptimizationEnabled())
{
// No overflow arithmetic with pointers
noway_assert(!op1->gtOverflow());

temp = op1->AsOp()->gtOp1->AsOp()->gtOp1;
if (!temp->OperIsLocal())
{
temp = nullptr;
break;
}

// Can not remove the GT_ADDR if it is currently a CSE candidate.
if (gtIsActiveCSE_Candidate(op1->AsOp()->gtOp1))
{
break;
}

ival1 = op1->AsOp()->gtOp2->AsIntCon()->gtIconVal;
fieldSeq = op1->AsOp()->gtOp2->AsIntCon()->gtFieldSeq;

if (ival1 == 0 && typ == temp->TypeGet() && temp->TypeGet() != TYP_STRUCT)
{
noway_assert(!varTypeIsGC(temp->TypeGet()));
foldAndReturnTemp = true;
}
else
{
// The emitter can't handle large offsets
if (ival1 != (unsigned short)ival1)
{
break;
}

// The emitter can get confused by invalid offsets
if (ival1 >= Compiler::lvaLclSize(temp->AsLclVarCommon()->GetLclNum()))
{
break;
}
}
// Now we can fold this into a GT_LCL_FLD below
// where we check (temp != nullptr)
}
}
}

// At this point we may have a lclVar or lclFld that might be foldable with a bit of extra massaging:
// - We may have a load of a local where the load has a different type than the local
// - We may have a load of a local plus an offset
//
// In these cases, we will change the lclVar or lclFld into a lclFld of the appropriate type and
// offset if doing so is legal. The only cases in which this transformation is illegal are if the load
// begins before the local or if the load extends beyond the end of the local (i.e. if the load is
// out-of-bounds w.r.t. the local).
// At this point we may have a lclVar or lclFld of some mismatched type. In this case, we will change
// the lclVar or lclFld into a lclFld of the appropriate type if doing so is legal. The only cases in
// which this transformation is illegal is when we have a STRUCT indirection, as we do not have the
// necessary layout information, or if the load would extend beyond the local.
if ((temp != nullptr) && !foldAndReturnTemp)
{
assert(temp->OperIsLocal());
assert(temp->OperIs(GT_LCL_VAR, GT_LCL_FLD));

unsigned lclNum = temp->AsLclVarCommon()->GetLclNum();
unsigned lclNum = temp->AsLclVarCommon()->GetLclNum();
unsigned lclOffs = temp->AsLclVarCommon()->GetLclOffs();

// Make sure we do not enregister this lclVar.
lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::LocalField));

// If the size of the load is greater than the size of the lclVar, we cannot fold this access into
// a lclFld: the access represented by an lclFld node must begin at or after the start of the
// lclVar and must not extend beyond the end of the lclVar.
if ((ival1 >= 0) && ((ival1 + genTypeSize(typ)) <= lvaLclExactSize(lclNum)))
if ((typ != TYP_STRUCT) && ((lclOffs + genTypeSize(typ)) <= lvaLclExactSize(lclNum)))
{
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.
// We will change the type of the node to match the orginal GT_IND type.
//
if (temp->OperGet() == GT_LCL_FLD)
{
lclFld = temp->AsLclFld();
lclFld->SetLclOffs(lclFld->GetLclOffs() + static_cast<unsigned>(ival1));
}
else // We have a GT_LCL_VAR.
temp->gtType = typ;

if (temp->OperIs(GT_LCL_VAR))
{
assert(temp->OperGet() == GT_LCL_VAR);
temp->ChangeOper(GT_LCL_FLD); // Note that this makes the gtFieldSeq "NotAField".
lclFld = temp->AsLclFld();
lclFld->SetLclOffs(static_cast<unsigned>(ival1));
temp->ChangeOper(GT_LCL_FLD);
}

temp->gtType = tree->gtType;
foldAndReturnTemp = true;
}
}
Expand All @@ -11910,20 +11830,15 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
{
assert(temp != nullptr);
assert(temp->TypeGet() == typ);
assert((op1->OperGet() == GT_ADD) || (op1->OperGet() == GT_ADDR));
assert(op1->OperIs(GT_ADDR));

// Copy the value of GTF_DONT_CSE from the original tree to `temp`: it can be set for
// 'temp' because a GT_ADDR always marks it for its operand.
temp->gtFlags &= ~GTF_DONT_CSE;
temp->gtFlags |= (tree->gtFlags & GTF_DONT_CSE);
temp->SetVNsFromNode(tree);

if (op1->OperGet() == GT_ADD)
{
DEBUG_DESTROY_NODE(op1->AsOp()->gtOp1); // GT_ADDR
DEBUG_DESTROY_NODE(op1->AsOp()->gtOp2); // GT_CNS_INT
}
DEBUG_DESTROY_NODE(op1); // GT_ADD or GT_ADDR
DEBUG_DESTROY_NODE(op1); // GT_ADDR
DEBUG_DESTROY_NODE(tree); // GT_IND

// If the result of the fold is a local var, we may need to perform further adjustments e.g. for
Expand Down Expand Up @@ -12925,9 +12840,39 @@ GenTree* Compiler::fgOptimizeAddition(GenTreeOp* add)
return op1;
}

// Note that these transformations are legal for floating-point ADDs as well.
if (opts.OptimizationEnabled())
{
// Reduce local addresses: "ADD(ADDR(LCL_VAR), OFFSET)" => "ADDR(LCL_FLD OFFSET)".
// TODO-ADDR: do "ADD(LCL_FLD/VAR_ADDR, OFFSET)" => "LCL_FLD_ADDR" instead.
//
if (op1->OperIs(GT_ADDR) && op2->IsCnsIntOrI() && op1->gtGetOp1()->OperIsLocalRead())
{
GenTreeUnOp* addrNode = op1->AsUnOp();
GenTreeLclVarCommon* lclNode = addrNode->gtGetOp1()->AsLclVarCommon();
GenTreeIntCon* offsetNode = op2->AsIntCon();
if (FitsIn<uint16_t>(offsetNode->IconValue()))
{
unsigned offset = lclNode->GetLclOffs() + static_cast<uint16_t>(offsetNode->IconValue());

// Note: the emitter does not expect out-of-bounds access for LCL_FLD_ADDR.
if (FitsIn<uint16_t>(offset) && (offset < lvaLclExactSize(lclNode->GetLclNum())))
{
// Types of location nodes under ADDRs do not matter. We arbitrarily choose TYP_UBYTE.
lclNode->ChangeType(TYP_UBYTE);
lclNode->SetOper(GT_LCL_FLD);
lclNode->AsLclFld()->SetLclOffs(offset);
lvaSetVarDoNotEnregister(lclNode->GetLclNum() DEBUGARG(DoNotEnregisterReason::LocalField));

addrNode->SetVNsFromNode(add);

DEBUG_DESTROY_NODE(offsetNode);
DEBUG_DESTROY_NODE(add);

return addrNode;
}
}
}

// - a + b = > b - a
// ADD((NEG(a), b) => SUB(b, a)

Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8478,8 +8478,7 @@ void Compiler::fgValueNumberTree(GenTree* tree)
{
unsigned lclNum = lclFld->GetLclNum();

// TODO-ADDR: delete the "GetSize" check once location nodes are no more.
if (!lvaInSsa(lclFld->GetLclNum()) || !lclFld->HasSsaName() || (lclFld->GetSize() == 0))
if (!lvaInSsa(lclFld->GetLclNum()) || !lclFld->HasSsaName())
{
lclFld->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclFld->TypeGet()));
}
Expand Down