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

Jump to bottom

Transform local indirections off of TYP_BLK locals; support numbering exposed LCL_FLDs #79771

Merged
merged 3 commits into from
Jan 11, 2023
Merged

Transform local indirections off of TYP_BLK locals; support numbering exposed LCL_FLDs #79771

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
801816c
Transform local indirections off of TYP_BLK locals
SingleAccretion Dec 16, 2022
225eb65
Support numbering exposed LCL_FLDs
SingleAccretion Dec 16, 2022
040c4da
Fix an optimizer bug
SingleAccretion Jan 4, 2023
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
37 changes: 16 additions & 21 deletions src/coreclr/jit/lclmorph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,20 +687,21 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
else
{
// Otherwise it must be accessed through some kind of indirection. Usually this is
// something like IND(ADDR(LCL_VAR)), global morph will change it to GT_LCL_VAR or
// GT_LCL_FLD so the lclvar does not need to be address exposed.
// something like IND(LCL_ADDR_VAR), which we will change to LCL_VAR or LCL_FLD so
// the lclvar does not need to be address exposed.
//
// However, it is possible for the indirection to be wider than the lclvar
// (e.g. *(long*)&int32Var) or to have a field offset that pushes the indirection
// past the end of the lclvar memory location. In such cases morph doesn't do
// anything so the lclvar needs to be address exposed.
// past the end of the lclvar memory location. In such cases we cannot do anything
// so the lclvar needs to be address exposed.
//
// More importantly, if the lclvar is a promoted struct field then the parent lclvar
// also needs to be address exposed so we get dependent struct promotion. Code like
// *(long*)&int32Var has undefined behavior and it's practically useless but reading,
// say, 2 consecutive Int32 struct fields as Int64 has more practical value.

LclVarDsc* varDsc = m_compiler->lvaGetDesc(val.LclNum());
unsigned lclNum = val.LclNum();
LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum);
unsigned indirSize = GetIndirSize(node);
bool isWide;

Expand All @@ -719,24 +720,18 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
{
isWide = true;
}
else if (varDsc->TypeGet() == TYP_STRUCT)
{
isWide = (endOffset.Value() > varDsc->lvExactSize);
}
else
{
// For small int types use the real type size, not the stack slot size.
// Morph does manage to transform `*(int*)&byteVar` into just byteVar where
// the LCL_VAR node has type TYP_INT. But such code is simply bogus and
// there's no reason to attempt to optimize it. It makes more sense to
// mark the variable address exposed in such circumstances.
//
// Same for "small" SIMD types - SIMD8/12 have 8/12 bytes, even if the
// stack location may have 16 bytes.
//
// For TYP_BLK variables the type size is 0 so they're always address
// exposed.
isWide = (endOffset.Value() > genTypeSize(varDsc->TypeGet()));
isWide = endOffset.Value() > m_compiler->lvaLclExactSize(lclNum);

if (varDsc->TypeGet() == TYP_BLK)
{
// TODO-CQ: TYP_BLK used to always be exposed here. This is in principle not necessary, but
// not doing so would require VN changes. For now, exposing gets better CQ as otherwise the
// variable ends up untracked and VN treats untracked-not-exposed locals more conservatively
// than exposed ones.
m_compiler->lvaSetVarAddrExposed(lclNum DEBUGARG(AddressExposedReason::TOO_CONSERVATIVE));
}
}
}

Expand Down
9 changes: 4 additions & 5 deletions src/coreclr/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8601,13 +8601,12 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
}
}
// Otherwise, must be local lhs form. I should assert that.
else if (lhs->OperGet() == GT_LCL_VAR)
else if (lhs->OperIsLocal())
{
GenTreeLclVar* lhsLcl = lhs->AsLclVar();
GenTree* rhs = tree->AsOp()->gtOp2;
ValueNum rhsVN = rhs->gtVNPair.GetLiberal();
GenTreeLclVarCommon* lhsLcl = lhs->AsLclVarCommon();
ValueNum rhsVN = tree->AsOp()->gtOp2->gtVNPair.GetLiberal();
// If we gave the RHS a value number, propagate it.
if (rhsVN != ValueNumStore::NoVN)
if (lhsLcl->OperIs(GT_LCL_VAR) && (rhsVN != ValueNumStore::NoVN))
{
rhsVN = vnStore->VNNormalValue(rhsVN);
if (lhsLcl->HasSsaName())
Expand Down
25 changes: 18 additions & 7 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8846,20 +8846,31 @@ void Compiler::fgValueNumberTree(GenTree* tree)
// If this is a (full or partial) def we skip; it will be handled as part of the assignment.
if ((lclFld->gtFlags & GTF_VAR_DEF) == 0)
{
unsigned lclNum = lclFld->GetLclNum();
unsigned lclNum = lclFld->GetLclNum();
LclVarDsc* varDsc = lvaGetDesc(lclNum);

if (!lclFld->HasSsaName())
if (lclFld->HasSsaName())
{
lclFld->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclFld->TypeGet()));
}
else
{
LclVarDsc* varDsc = lvaGetDesc(lclNum);
ValueNumPair lclVarValue = varDsc->GetPerSsaData(lclFld->GetSsaNum())->m_vnPair;
lclFld->gtVNPair =
vnStore->VNPairForLoad(lclVarValue, lvaLclExactSize(lclNum), lclFld->TypeGet(),
lclFld->GetLclOffs(), lclFld->GetSize());
}
else if (varDsc->IsAddressExposed())
{
// Address-exposed locals are part of ByrefExposed.
ValueNum addrVN = vnStore->VNForFunc(TYP_BYREF, VNF_PtrToLoc, vnStore->VNForIntCon(lclNum),
vnStore->VNForIntPtrCon(lclFld->GetLclOffs()));
ValueNum loadVN = fgValueNumberByrefExposedLoad(lclFld->TypeGet(), addrVN);

lclFld->gtVNPair.SetLiberal(loadVN);
lclFld->gtVNPair.SetConservative(vnStore->VNForExpr(compCurBB, lclFld->TypeGet()));
}
else
{
// An untracked local, and other odd cases.
lclFld->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclFld->TypeGet()));
}
}
else
{
Expand Down