Skip to content

Keep correct fieldSeq for 0-offset fields. #32085

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
sandreenko merged 5 commits into from
Feb 15, 2020
Merged

Keep correct fieldSeq for 0-offset fields. #32085

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
04a9f02
a few renamings in fgMorphCopyBlock.
Feb 7, 2020
e3f0fe0
Add zero field offsets where it was missed.
Feb 13, 2020
e820fa8
Add an optimization to lower to delete `LEA(addr, 0)`.
Feb 13, 2020
b6a9008
Fix for x86 tests.
Feb 14, 2020
18962c9
Add a function header for `LowerNode`.
Feb 14, 2020
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
Next Next commit
a few renamings in fgMorphCopyBlock. 
Clean up the existing code.
  • Loading branch information
Sergey Andreenko committed Feb 13, 2020
commit 04a9f025b07eb8a78c64611e124f2073b0c20563
134 changes: 64 additions & 70 deletions src/coreclr/src/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9714,12 +9714,12 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
bool isLateArg = (tree->gtFlags & GTF_LATE_ARG) != 0;

GenTree* asg = tree;
GenTree* rhs = asg->gtGetOp2();
GenTree* src = asg->gtGetOp2();
GenTree* dest = asg->gtGetOp1();

#if FEATURE_MULTIREG_RET
// If this is a multi-reg return, we will not do any morphing of this node.
if (rhs->IsMultiRegCall())
if (src->IsMultiRegCall())
{
assert(dest->OperGet() == GT_LCL_VAR);
JITDUMP(" not morphing a multireg call return\n");
Expand All @@ -9739,9 +9739,9 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
}
}
asg->gtType = dest->TypeGet();
rhs = fgMorphBlkNode(rhs, false);
src = fgMorphBlkNode(src, false);

asg->AsOp()->gtOp2 = rhs;
asg->AsOp()->gtOp2 = src;

GenTree* oldTree = tree;
GenTree* oneAsgTree = fgMorphOneAsgBlockOp(tree);
Expand Down Expand Up @@ -9877,24 +9877,24 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
LclVarDsc* srcLclVar = nullptr;
bool srcDoFldAsg = false;

if (rhs->IsLocal())
if (src->IsLocal())
{
srcLclVarTree = rhs->AsLclVarCommon();
srcLclVarTree = src->AsLclVarCommon();
srcLclNum = srcLclVarTree->GetLclNum();
if (rhs->OperGet() == GT_LCL_FLD)
if (src->OperGet() == GT_LCL_FLD)
{
srcFldSeq = rhs->AsLclFld()->GetFieldSeq();
srcFldSeq = src->AsLclFld()->GetFieldSeq();
}
}
else if (rhs->OperIsIndir())
else if (src->OperIsIndir())
{
if (rhs->AsOp()->gtOp1->IsLocalAddrExpr(this, &srcLclVarTree, &srcFldSeq))
if (src->AsOp()->gtOp1->IsLocalAddrExpr(this, &srcLclVarTree, &srcFldSeq))
{
srcLclNum = srcLclVarTree->GetLclNum();
}
else
{
srcAddr = rhs->AsOp()->gtOp1;
srcAddr = src->AsOp()->gtOp1;
}
}

Expand Down Expand Up @@ -9961,9 +9961,9 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
}

#if defined(TARGET_ARM)
if ((rhs->OperIsIndir()) && (rhs->gtFlags & GTF_IND_UNALIGNED))
if ((src->OperIsIndir()) && (src->gtFlags & GTF_IND_UNALIGNED))
{
JITDUMP(" rhs is unaligned");
JITDUMP(" src is unaligned");
requiresCopyBlock = true;
}

Expand All @@ -9975,7 +9975,7 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
#endif // TARGET_ARM

// Can't use field by field assignment if the src is a call.
if (rhs->OperGet() == GT_CALL)
if (src->OperGet() == GT_CALL)
{
JITDUMP(" src is a call");
// C++ style CopyBlock with holes
Expand Down Expand Up @@ -10116,9 +10116,9 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
asg->AsOp()->gtOp1 = dest;
asg->gtFlags |= (dest->gtFlags & GTF_ALL_EFFECT);

// Eliminate the "OBJ or BLK" node on the rhs.
rhs = fgMorphBlockOperand(rhs, asgType, blockWidth, false /*!isBlkReqd*/);
asg->AsOp()->gtOp2 = rhs;
// Eliminate the "OBJ or BLK" node on the src.
src = fgMorphBlockOperand(src, asgType, blockWidth, false /*!isBlkReqd*/);
asg->AsOp()->gtOp2 = src;

goto _Done;
}
Expand All @@ -10128,7 +10128,6 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
//
tree = nullptr;

GenTree* src;
GenTree* addrSpill = nullptr;
unsigned addrSpillTemp = BAD_VAR_NUM;
bool addrSpillIsStackDest = false; // true if 'addrSpill' represents the address in our local stack frame
Expand All @@ -10149,10 +10148,10 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
else if (destDoFldAsg)
{
fieldCnt = destLclVar->lvFieldCnt;
rhs = fgMorphBlockOperand(rhs, asgType, blockWidth, false /*isBlkReqd*/);
src = fgMorphBlockOperand(src, asgType, blockWidth, false /*isBlkReqd*/);
if (srcAddr == nullptr)
{
srcAddr = fgMorphGetStructAddr(&rhs, destLclVar->lvVerTypeInfo.GetClassHandle(), true /* rValue */);
srcAddr = fgMorphGetStructAddr(&src, destLclVar->lvVerTypeInfo.GetClassHandle(), true /* rValue */);
}
}
else
Expand Down Expand Up @@ -10282,57 +10281,55 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
// So, beyond this point we cannot rely on the old values of 'srcLclVar' and 'destLclVar'.
for (unsigned i = 0; i < fieldCnt; ++i)
{
FieldSeqNode* curFieldSeq = nullptr;
GenTree* dstFld;
if (destDoFldAsg)
{
noway_assert(destLclNum != BAD_VAR_NUM);
unsigned fieldLclNum = lvaTable[destLclNum].lvFieldLclStart + i;
dest = gtNewLclvNode(fieldLclNum, lvaTable[fieldLclNum].TypeGet());
unsigned dstFieldLclNum = lvaTable[destLclNum].lvFieldLclStart + i;
dstFld = gtNewLclvNode(dstFieldLclNum, lvaTable[dstFieldLclNum].TypeGet());
// If it had been labeled a "USEASG", assignments to the the individual promoted fields are not.
if (destAddr != nullptr)
{
noway_assert(destAddr->AsOp()->gtOp1->gtOper == GT_LCL_VAR);
dest->gtFlags |= destAddr->AsOp()->gtOp1->gtFlags & ~(GTF_NODE_MASK | GTF_VAR_USEASG);
dstFld->gtFlags |= destAddr->AsOp()->gtOp1->gtFlags & ~(GTF_NODE_MASK | GTF_VAR_USEASG);
}
else
{
noway_assert(lclVarTree != nullptr);
dest->gtFlags |= lclVarTree->gtFlags & ~(GTF_NODE_MASK | GTF_VAR_USEASG);
dstFld->gtFlags |= lclVarTree->gtFlags & ~(GTF_NODE_MASK | GTF_VAR_USEASG);
}
// Don't CSE the lhs of an assignment.
dest->gtFlags |= GTF_DONT_CSE;
dstFld->gtFlags |= GTF_DONT_CSE;
}
else
{
noway_assert(srcDoFldAsg);
noway_assert(srcLclNum != BAD_VAR_NUM);
unsigned fieldLclNum = lvaTable[srcLclNum].lvFieldLclStart + i;

if (destSingleLclVarAsg)
{
noway_assert(fieldCnt == 1);
noway_assert(destLclVar != nullptr);
noway_assert(addrSpill == nullptr);

dest = gtNewLclvNode(destLclNum, destLclVar->TypeGet());
dstFld = gtNewLclvNode(destLclNum, destLclVar->TypeGet());
}
else
{
if (addrSpill)
{
assert(addrSpillTemp != BAD_VAR_NUM);
dest = gtNewLclvNode(addrSpillTemp, TYP_BYREF);
dstFld = gtNewLclvNode(addrSpillTemp, TYP_BYREF);
}
else
{
dest = gtCloneExpr(destAddr);
noway_assert(dest != nullptr);
dstFld = gtCloneExpr(destAddr);
noway_assert(dstFld != nullptr);

// Is the address of a local?
GenTreeLclVarCommon* lclVarTree = nullptr;
bool isEntire = false;
bool* pIsEntire = (blockWidthIsConst ? &isEntire : nullptr);
if (dest->DefinesLocalAddr(this, blockWidth, &lclVarTree, pIsEntire))
if (dstFld->DefinesLocalAddr(this, blockWidth, &lclVarTree, pIsEntire))
{
lclVarTree->gtFlags |= GTF_VAR_DEF;
if (!isEntire)
Expand All @@ -10342,68 +10339,72 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
}
}

GenTree* fieldOffsetNode = gtNewIconNode(lvaTable[fieldLclNum].lvFldOffset, TYP_I_IMPL);
LclVarDsc* srcVarDsc = lvaGetDesc(srcLclNum);
unsigned srcFieldLclNum = srcVarDsc->lvFieldLclStart + i;
LclVarDsc* srcFieldVarDsc = lvaGetDesc(srcFieldLclNum);

// Have to set the field sequence -- which means we need the field handle.
CORINFO_CLASS_HANDLE classHnd = lvaTable[srcLclNum].lvVerTypeInfo.GetClassHandle();
CORINFO_CLASS_HANDLE classHnd = srcVarDsc->lvVerTypeInfo.GetClassHandle();
CORINFO_FIELD_HANDLE fieldHnd =
info.compCompHnd->getFieldInClass(classHnd, lvaTable[fieldLclNum].lvFldOrdinal);
curFieldSeq = GetFieldSeqStore()->CreateSingleton(fieldHnd);
fieldOffsetNode->AsIntCon()->gtFieldSeq = curFieldSeq;
info.compCompHnd->getFieldInClass(classHnd, srcFieldVarDsc->lvFldOrdinal);
FieldSeqNode* curFieldSeq = GetFieldSeqStore()->CreateSingleton(fieldHnd);
unsigned srcFieldOffset = lvaGetDesc(srcFieldLclNum)->lvFldOffset;
GenTree* fieldOffsetNode = gtNewIconNode(srcFieldVarDsc->lvFldOffset, curFieldSeq);

dest = gtNewOperNode(GT_ADD, TYP_BYREF, dest, fieldOffsetNode);

dest = gtNewIndir(lvaTable[fieldLclNum].TypeGet(), dest);
dstFld = gtNewOperNode(GT_ADD, TYP_BYREF, dstFld, fieldOffsetNode);
dstFld = gtNewIndir(srcFieldVarDsc->TypeGet(), dstFld);

// !!! The destination could be on stack. !!!
// This flag will let us choose the correct write barrier.
dest->gtFlags |= GTF_IND_TGTANYWHERE;
dstFld->gtFlags |= GTF_IND_TGTANYWHERE;
}
}

GenTree* srcFld;
if (srcDoFldAsg)
{
noway_assert(srcLclNum != BAD_VAR_NUM);
unsigned fieldLclNum = lvaTable[srcLclNum].lvFieldLclStart + i;
src = gtNewLclvNode(fieldLclNum, lvaTable[fieldLclNum].TypeGet());
unsigned srcFieldLclNum = lvaTable[srcLclNum].lvFieldLclStart + i;
srcFld = gtNewLclvNode(srcFieldLclNum, lvaTable[srcFieldLclNum].TypeGet());

noway_assert(srcLclVarTree != nullptr);
src->gtFlags |= srcLclVarTree->gtFlags & ~GTF_NODE_MASK;
srcFld->gtFlags |= srcLclVarTree->gtFlags & ~GTF_NODE_MASK;
}
else
{
noway_assert(destDoFldAsg);
noway_assert(destLclNum != BAD_VAR_NUM);
unsigned fieldLclNum = lvaTable[destLclNum].lvFieldLclStart + i;
unsigned dstFieldLclNum = lvaTable[destLclNum].lvFieldLclStart + i;

if (srcSingleLclVarAsg)
{
noway_assert(fieldCnt == 1);
noway_assert(srcLclNum != BAD_VAR_NUM);
noway_assert(addrSpill == nullptr);

src = gtNewLclvNode(srcLclNum, lvaGetDesc(srcLclNum)->TypeGet());
srcFld = gtNewLclvNode(srcLclNum, lvaGetDesc(srcLclNum)->TypeGet());
}
else
{
if (addrSpill)
{
assert(addrSpillTemp != BAD_VAR_NUM);
src = gtNewLclvNode(addrSpillTemp, TYP_BYREF);
srcFld = gtNewLclvNode(addrSpillTemp, TYP_BYREF);
}
else
{
src = gtCloneExpr(srcAddr);
noway_assert(src != nullptr);
srcFld = gtCloneExpr(srcAddr);
noway_assert(srcFld != nullptr);
}

CORINFO_CLASS_HANDLE classHnd = lvaTable[destLclNum].lvVerTypeInfo.GetClassHandle();
CORINFO_FIELD_HANDLE fieldHnd =
info.compCompHnd->getFieldInClass(classHnd, lvaTable[fieldLclNum].lvFldOrdinal);
curFieldSeq = GetFieldSeqStore()->CreateSingleton(fieldHnd);
var_types destType = lvaGetDesc(fieldLclNum)->lvType;
info.compCompHnd->getFieldInClass(classHnd, lvaTable[dstFieldLclNum].lvFldOrdinal);
FieldSeqNode* curFieldSeq = GetFieldSeqStore()->CreateSingleton(fieldHnd);
var_types destType = lvaGetDesc(dstFieldLclNum)->lvType;

bool done = false;
if (lvaGetDesc(fieldLclNum)->lvFldOffset == 0)
if (lvaGetDesc(dstFieldLclNum)->lvFldOffset == 0)
{
// If this is a full-width use of the src via a different type, we need to create a GT_LCL_FLD.
// (Note that if it was the same type, 'srcSingleLclVarAsg' would be true.)
Expand All @@ -10421,28 +10422,28 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
srcLclVarTree->ChangeOper(GT_LCL_FLD);
srcLclVarTree->gtType = destType;
srcLclVarTree->AsLclFld()->SetFieldSeq(curFieldSeq);
src = srcLclVarTree;
done = true;
srcFld = srcLclVarTree;
done = true;
}
}
}
else // if (lvaGetDesc(fieldLclNum)->lvFldOffset != 0)
{
src = gtNewOperNode(GT_ADD, TYP_BYREF, src,
new (this, GT_CNS_INT)
GenTreeIntCon(TYP_I_IMPL, lvaGetDesc(fieldLclNum)->lvFldOffset,
curFieldSeq));
srcFld = gtNewOperNode(GT_ADD, TYP_BYREF, srcFld,
new (this, GT_CNS_INT)
GenTreeIntCon(TYP_I_IMPL, lvaGetDesc(dstFieldLclNum)->lvFldOffset,
curFieldSeq));
}
if (!done)
{
src = gtNewIndir(destType, src);
srcFld = gtNewIndir(destType, srcFld);
}
}
}

noway_assert(dest->TypeGet() == src->TypeGet());
noway_assert(dstFld->TypeGet() == srcFld->TypeGet());

asg = gtNewAssignNode(dest, src);
asg = gtNewAssignNode(dstFld, srcFld);

// If we spilled the address, and we didn't do individual field assignments to promoted fields,
// and it was of a local, ensure that the destination local variable has been marked as address
Expand Down Expand Up @@ -13263,13 +13264,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)

if (isZeroOffset)
{
// 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
zeroFieldSeq = GetFieldSeqStore()->Append(existingZeroOffsetFldSeq, zeroFieldSeq);
}
// Transfer the annotation to the new GT_ADDR node.
fgAddFieldSeqForZeroOffset(op1, zeroFieldSeq);
Copy link
Contributor

@CarolEidt CarolEidt Feb 13, 2020

Choose a reason for hiding this comment

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

Should there be an assert here that zeroFieldSeq != nullptr or is that self-evident from the fact that isZeroOffset is true?

Copy link
Contributor Author

@sandreenko sandreenko Feb 13, 2020

Choose a reason for hiding this comment

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

We could add it, but it looks self-evident.
Both values are initialized a few lines above with:
bool isZeroOffset = GetZeroOffsetFieldMap()->Lookup(tree, &zeroFieldSeq);
so isZeroOffset == true gurantees zeroFieldSeq != nullptr.

The problem with that code was that the condition was written before fgAddFieldSeqForZeroOffset was introduced. When fgAddFieldSeqForZeroOffset was created that logic was encapsulated there, but the condition was not deleted.

Copy link
Contributor

@CarolEidt CarolEidt Feb 13, 2020

Choose a reason for hiding this comment

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

Thanks for the clarification

}
Expand Down