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

Struct & SIMD improvements #22255

Merged
merged 4 commits into from
Mar 28, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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: 1 addition & 9 deletions src/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4529,15 +4529,7 @@ void CodeGen::genCheckUseBlockInit()
unless they are untracked GC type or structs that contain GC pointers */
CLANG_FORMAT_COMMENT_ANCHOR;

#if FEATURE_SIMD
// TODO-1stClassStructs
// This is here to duplicate previous behavior, where TYP_SIMD8 locals
// were not being re-typed correctly.
if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT) || (varDsc->lvType == TYP_SIMD8)) &&
#else // !FEATURE_SIMD
if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT)) &&
#endif // !FEATURE_SIMD
varDsc->lvOnFrame &&
if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT)) && varDsc->lvOnFrame &&
(!varDsc->lvIsTemp || varTypeIsGC(varDsc->TypeGet()) || (varDsc->lvStructGcCount > 0)))
{
varDsc->lvMustInit = true;
Expand Down
30 changes: 30 additions & 0 deletions src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16509,15 +16509,45 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
if (varTypeIsSIMD(tree))
{
structHnd = gtGetStructHandleForSIMD(tree->gtType, TYP_FLOAT);
#ifdef FEATURE_HW_INTRINSICS
if (structHnd == NO_CLASS_HANDLE)
{
structHnd = gtGetStructHandleForHWSIMD(tree->gtType, TYP_FLOAT);
}
#endif
}
else
#endif
{
// Attempt to find a handle for this expression.
// We can do this for an array element indirection, or for a field indirection.
ArrayInfo arrInfo;
if (TryGetArrayInfo(tree->AsIndir(), &arrInfo))
{
structHnd = EncodeElemType(arrInfo.m_elemType, arrInfo.m_elemStructType);
}
else
{
GenTree* addr = tree->AsIndir()->Addr();
if ((addr->OperGet() == GT_ADD) && addr->gtGetOp2()->OperIs(GT_CNS_INT))
{
FieldSeqNode* fieldSeq = addr->gtGetOp2()->AsIntCon()->gtFieldSeq;

if (fieldSeq != nullptr)
{
while (fieldSeq->m_next != nullptr)
{
fieldSeq = fieldSeq->m_next;
}
if (fieldSeq != FieldSeqStore::NotAField() && !fieldSeq->IsPseudoField())
{
CORINFO_FIELD_HANDLE fieldHnd = fieldSeq->m_fieldHnd;
CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &structHnd);
assert(fieldCorType == CORINFO_TYPE_VALUECLASS);
}
Copy link
Member

Choose a reason for hiding this comment

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

There is similar handle searching logic in gtGetClassHandle -- perhaps factor this out as a helper method?

Copy link
Author

Choose a reason for hiding this comment

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

They're really quite different, and though some logic could be sharable, the ref context seems quite different from the value-type context.

Copy link

Choose a reason for hiding this comment

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

Can't we just get rid of this completly? It seems preferrable to keep GT_OBJ nodes around and extract the struct handle from those instead of attempting to recover handles from GT_IND this way.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that we'd like to get rid of this in the long term, but this code is to deal with cases where we have already made that transformation. I'm going to look into this a bit further, so that I better understand the cases that require this.

Copy link

Choose a reason for hiding this comment

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

Yes, my comment was targeted at long term. Well, maybe not that long, just only after I'm done with #21705 that makes GT_OBJ node "small" and overall cheap.

}
}
}
}
break;
#ifdef FEATURE_SIMD
Expand Down
22 changes: 14 additions & 8 deletions src/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -4712,22 +4712,28 @@ struct GenTreeObj : public GenTreeBlk
}
}

void Init()
{
// By default, an OBJ is assumed to be a global reference, unless it is local.
GenTreeLclVarCommon* lcl = Addr()->IsLocalAddrExpr();
if ((lcl == nullptr) || ((lcl->gtFlags & GTF_GLOB_EFFECT) != 0))
{
gtFlags |= GTF_GLOB_REF;
}
noway_assert(gtClass != NO_CLASS_HANDLE);
_gtGcPtrCount = UINT32_MAX;
}

GenTreeObj(var_types type, GenTree* addr, CORINFO_CLASS_HANDLE cls, unsigned size)
: GenTreeBlk(GT_OBJ, type, addr, size), gtClass(cls)
{
// By default, an OBJ is assumed to be a global reference.
gtFlags |= GTF_GLOB_REF;
noway_assert(cls != NO_CLASS_HANDLE);
_gtGcPtrCount = UINT32_MAX;
Init();
}

GenTreeObj(var_types type, GenTree* addr, GenTree* data, CORINFO_CLASS_HANDLE cls, unsigned size)
: GenTreeBlk(GT_STORE_OBJ, type, addr, data, size), gtClass(cls)
{
// By default, an OBJ is assumed to be a global reference.
gtFlags |= GTF_GLOB_REF;
noway_assert(cls != NO_CLASS_HANDLE);
_gtGcPtrCount = UINT32_MAX;
Init();
}

#if DEBUGGABLE_GENTREE
Expand Down
30 changes: 3 additions & 27 deletions src/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -618,21 +618,8 @@ inline void Compiler::impAppendStmt(GenTree* stmt, unsigned chkLevel)
// Since we don't know what it assigns to, we need to spill global refs.
spillGlobEffects = true;
}
else if (!expr->OperIsBlkOp())
else if ((lhs->gtFlags & GTF_GLOB_REF) != 0)
{
// If we are assigning to a global ref, we have to spill global refs on stack
if ((lhs->gtFlags & GTF_GLOB_REF) != 0)
{
spillGlobEffects = true;
}
}
else if ((lhs->OperIsBlk() && !lhs->AsBlk()->HasGCPtr()) ||
((lhs->OperGet() == GT_LCL_VAR) &&
(lvaTable[lhs->AsLclVarCommon()->gtLclNum].lvStructGcCount == 0)))
{
// TODO-1stClassStructs: Previously, spillGlobEffects was set to true for
// GT_INITBLK and GT_COPYBLK, but this is overly conservative, and should be
// revisited. (Note that it was NOT set to true for GT_COPYOBJ.)
Copy link
Author

Choose a reason for hiding this comment

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

This was where we were previously always spilling on a non-COPYOBJ block assignment. This was (clearly) overly conservative, but by eliminating this conservatism, I failed to add the struct case to the code in impImportBlockCode (at the SPILL_APPEND: label in importer.cpp) (bug #23545, fixed in PR #23570)

spillGlobEffects = true;
}
}
Expand Down Expand Up @@ -1389,8 +1376,6 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
}
if (dest == nullptr)
{
// TODO-1stClassStructs: We shouldn't really need a block node as the destination
// if this is a known struct type.
if (asgType == TYP_STRUCT)
{
dest = gtNewObjNode(structHnd, destAddr);
Expand All @@ -1401,10 +1386,6 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
dest->gtFlags &= ~GTF_GLOB_REF;
dest->gtFlags |= (destAddr->gtFlags & GTF_GLOB_REF);
}
else if (varTypeIsStruct(asgType))
{
dest = new (this, GT_BLK) GenTreeBlk(GT_BLK, asgType, destAddr, genTypeSize(asgType));
}
else
{
dest = gtNewOperNode(GT_IND, asgType, destAddr);
Expand Down Expand Up @@ -14527,9 +14508,8 @@ void Compiler::impImportBlockCode(BasicBlock* block)
assert(!"Unexpected fieldAccessor");
}

// Create the member assignment, unless we have a struct.
// TODO-1stClassStructs: This could be limited to TYP_STRUCT, to avoid extra copies.
bool deferStructAssign = varTypeIsStruct(lclTyp);
// Create the member assignment, unless we have a TYP_STRUCT.
bool deferStructAssign = (lclTyp == TYP_STRUCT);

if (!deferStructAssign)
{
Expand Down Expand Up @@ -16220,10 +16200,6 @@ GenTree* Compiler::impAssignSmallStructTypeToVar(GenTree* op, CORINFO_CLASS_HAND
unsigned tmpNum = lvaGrabTemp(true DEBUGARG("Return value temp for small struct return."));
impAssignTempGen(tmpNum, op, hClass, (unsigned)CHECK_SPILL_ALL);
GenTree* ret = gtNewLclvNode(tmpNum, lvaTable[tmpNum].lvType);

// TODO-1stClassStructs: Handle constant propagation and CSE-ing of small struct returns.
ret->gtFlags |= GTF_DONT_CSE;

return ret;
}

Expand Down
5 changes: 3 additions & 2 deletions src/jit/lclvars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2170,6 +2170,9 @@ void Compiler::StructPromotionHelper::PromoteStructVar(unsigned lclNum)
// Set size to zero so that lvaSetStruct will appropriately set the SIMD-relevant fields.
fieldVarDsc->lvExactSize = 0;
compiler->lvaSetStruct(varNum, pFieldInfo->fldTypeHnd, false, true);
// We will not recursively promote this, so mark it as 'lvRegStruct' (note that we wouldn't
// be promoting this if we didn't think it could be enregistered.
fieldVarDsc->lvRegStruct = true;
}
#endif // FEATURE_SIMD

Expand Down Expand Up @@ -7043,8 +7046,6 @@ void Compiler::lvaDumpEntry(unsigned lclNum, FrameLayoutState curState, size_t r
printf(" V%02u.%s(offs=0x%02x)", varDsc->lvParentLcl, eeGetFieldName(fldHnd), varDsc->lvFldOffset);

lvaPromotionType promotionType = lvaGetPromotionType(parentvarDsc);
// We should never have lvIsStructField set if it is a reg-sized non-field-addressed struct.
assert(!varDsc->lvRegStruct);
switch (promotionType)
{
case PROMOTION_TYPE_NONE:
Expand Down
9 changes: 4 additions & 5 deletions src/jit/liveness.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2185,11 +2185,10 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
{
rhsNode = asgNode->AsBlk()->Data();
}
// TODO-1stClassStructs: There should be an else clause here to handle
// the non-block forms of store ops (GT_STORE_LCL_VAR, etc.) for which
// rhsNode is op1. (This isn't really a 1stClassStructs item, but the
// above was added to catch what used to be dead block ops, and that
// made this omission apparent.)
else
{
rhsNode = asgNode->gtGetOp2();
sandreenko marked this conversation as resolved.
Show resolved Hide resolved
}
}
else
{
Expand Down
6 changes: 1 addition & 5 deletions src/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1765,11 +1765,7 @@ void Lowering::CheckVSQuirkStackPaddingNeeded(GenTreeCall* call)
if (op1->OperGet() == GT_LCL_VAR_ADDR)
{
unsigned lclNum = op1->AsLclVarCommon()->GetLclNum();
// TODO-1stClassStructs: This is here to duplicate previous behavior,
// but is not needed because the scenario being quirked did not involve
// a SIMD or enregisterable struct.
// if(comp->lvaTable[lclNum].TypeGet() == TYP_STRUCT)
if (varTypeIsStruct(comp->lvaTable[lclNum].TypeGet()))
if (comp->lvaGetDesc(lclNum)->TypeGet() == TYP_STRUCT)
{
// First arg is addr of a struct local.
paddingNeeded = true;
Expand Down
Loading