Skip to content

Commit

Permalink
Don't retype struct as primitive types in import. (#33225)
Browse files Browse the repository at this point in the history 
* Add two tests.

* Don't retype struct returns x64 windows.

* Turn on for Windows x64 by default for tests.

* fix a few typos/leftovers.

* Disable for Win64 by default.

There are 2 issues that prevent it from being enbaled by default. They are causing significant asm regressions.
  • Loading branch information
Sergey Andreenko authored May 5, 2020
1 parent c9f9170 commit 5da855d
Show file tree
Hide file tree
Showing 14 changed files with 1,579 additions and 85 deletions.
5 changes: 5 additions & 0 deletions src/coreclr/src/jit/assertionprop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2679,6 +2679,11 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion,
newTree->ChangeOperConst(GT_CNS_INT);
newTree->AsIntCon()->gtIconVal = curAssertion->op2.u1.iconVal;
newTree->ClearIconHandleMask();
if (newTree->TypeIs(TYP_STRUCT))
{
// LCL_VAR can be init with a GT_CNS_INT, keep its type INT, not STRUCT.
newTree->ChangeType(TYP_INT);
}
}
// If we're doing an array index address, assume any constant propagated contributes to the index.
if (isArrIndex)
Expand Down
7 changes: 6 additions & 1 deletion src/coreclr/src/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2684,7 +2684,7 @@ class Compiler
fgArgTabEntry* gtArgEntryByLateArgIndex(GenTreeCall* call, unsigned lateArgInx);
static GenTree* gtArgNodeByLateArgInx(GenTreeCall* call, unsigned lateArgInx);

GenTree* gtNewAssignNode(GenTree* dst, GenTree* src);
GenTreeOp* gtNewAssignNode(GenTree* dst, GenTree* src);

GenTree* gtNewTempAssign(unsigned tmp,
GenTree* val,
Expand Down Expand Up @@ -9148,6 +9148,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#endif // !TARGET_AMD64
}

bool compDoOldStructRetyping()
{
return JitConfig.JitDoOldStructRetyping();
}

// Returns true if the method returns a value in more than one return register
// TODO-ARM-Bug: Deal with multi-register genReturnLocaled structs?
// TODO-ARM64: Does this apply for ARM64 too?
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/src/jit/compiler.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,7 @@ inline bool Compiler::VarTypeIsMultiByteAndCanEnreg(

if (varTypeIsStruct(type))
{
assert(typeClass != nullptr);
size = info.compCompHnd->getClassSize(typeClass);
if (forReturn)
{
Expand Down Expand Up @@ -1807,8 +1808,11 @@ inline void LclVarDsc::incRefCnts(BasicBlock::weight_t weight, Compiler* comp, R
//
// Increment counts on the local itself.
//
if (lvType != TYP_STRUCT || promotionType != Compiler::PROMOTION_TYPE_INDEPENDENT)
if ((lvType != TYP_STRUCT) || (promotionType != Compiler::PROMOTION_TYPE_INDEPENDENT))
{
// We increment ref counts of this local for primitive types, including structs that have been retyped as their
// only field, as well as for structs whose fields are not independently promoted.

//
// Increment lvRefCnt
//
Expand Down
22 changes: 20 additions & 2 deletions src/coreclr/src/jit/flowgraph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6052,7 +6052,14 @@ void Compiler::fgFindBasicBlocks()
{
// The lifetime of this var might expand multiple BBs. So it is a long lifetime compiler temp.
lvaInlineeReturnSpillTemp = lvaGrabTemp(false DEBUGARG("Inline return value spill temp"));
lvaTable[lvaInlineeReturnSpillTemp].lvType = info.compRetNativeType;
if (compDoOldStructRetyping())
{
lvaTable[lvaInlineeReturnSpillTemp].lvType = info.compRetNativeType;
}
else
{
lvaTable[lvaInlineeReturnSpillTemp].lvType = info.compRetType;
}

// If the method returns a ref class, set the class of the spill temp
// to the method's return value. We may update this later if it turns
Expand Down Expand Up @@ -8677,7 +8684,18 @@ class MergedReturns

if (comp->compMethodReturnsNativeScalarType())
{
returnLocalDsc.lvType = genActualType(comp->info.compRetNativeType);
if (!comp->compDoOldStructRetyping())
{
returnLocalDsc.lvType = genActualType(comp->info.compRetType);
if (varTypeIsStruct(returnLocalDsc.lvType))
{
comp->lvaSetStruct(returnLocalNum, comp->info.compMethodInfo->args.retTypeClass, false);
}
}
else
{
returnLocalDsc.lvType = genActualType(comp->info.compRetNativeType);
}
}
else if (comp->compMethodReturnsRetBufAddr())
{
Expand Down
83 changes: 57 additions & 26 deletions src/coreclr/src/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6481,7 +6481,7 @@ GenTree* Compiler::gtArgNodeByLateArgInx(GenTreeCall* call, unsigned lateArgInx)
* Create a node that will assign 'src' to 'dst'.
*/

GenTree* Compiler::gtNewAssignNode(GenTree* dst, GenTree* src)
GenTreeOp* Compiler::gtNewAssignNode(GenTree* dst, GenTree* src)
{
/* Mark the target as being assigned */

Expand All @@ -6498,7 +6498,7 @@ GenTree* Compiler::gtNewAssignNode(GenTree* dst, GenTree* src)

/* Create the assignment node */

GenTree* asg = gtNewOperNode(GT_ASG, dst->TypeGet(), dst, src);
GenTreeOp* asg = gtNewOperNode(GT_ASG, dst->TypeGet(), dst, src)->AsOp();

/* Mark the expression as containing an assignment */

Expand Down Expand Up @@ -15137,6 +15137,14 @@ GenTree* Compiler::gtNewTempAssign(
// and call returns. Lowering and Codegen will handle these.
ok = true;
}
else if ((dstTyp == TYP_STRUCT) && (valTyp == TYP_INT))
{
// It could come from `ASG(struct, 0)` that was propagated to `RETURN struct(0)`,
// and now it is merging to a struct again.
assert(!compDoOldStructRetyping());
assert(tmp == genReturnLocal);
ok = true;
}

if (!ok)
{
Expand Down Expand Up @@ -15165,31 +15173,51 @@ GenTree* Compiler::gtNewTempAssign(
// struct types. We don't have a convenient way to do that for all SIMD temps, since some
// internal trees use SIMD types that are not used by the input IL. In this case, we allow
// a null type handle and derive the necessary information about the type from its varType.
CORINFO_CLASS_HANDLE structHnd = gtGetStructHandleIfPresent(val);
if (varTypeIsStruct(varDsc) && ((structHnd != NO_CLASS_HANDLE) || (varTypeIsSIMD(valTyp))))
CORINFO_CLASS_HANDLE valStructHnd = gtGetStructHandleIfPresent(val);
if (varTypeIsStruct(varDsc) && (valStructHnd == NO_CLASS_HANDLE) && !varTypeIsSIMD(valTyp))
{
// There are 2 special cases:
// 1. we have lost classHandle from a FIELD node because the parent struct has overlapping fields,
// the field was transformed as IND opr GT_LCL_FLD;
// 2. we are propogating `ASG(struct V01, 0)` to `RETURN(struct V01)`, `CNT_INT` doesn't `structHnd`;
// in these cases, we can use the type of the merge return for the assignment.
assert(val->OperIs(GT_IND, GT_LCL_FLD, GT_CNS_INT));
assert(!compDoOldStructRetyping());
assert(tmp == genReturnLocal);
valStructHnd = lvaGetStruct(genReturnLocal);
assert(valStructHnd != NO_CLASS_HANDLE);
}

if ((valStructHnd != NO_CLASS_HANDLE) && val->IsConstInitVal())
{
assert(!compDoOldStructRetyping());
asg = gtNewAssignNode(dest, val);
}
else if (varTypeIsStruct(varDsc) && ((valStructHnd != NO_CLASS_HANDLE) || varTypeIsSIMD(valTyp)))
{
// The struct value may be be a child of a GT_COMMA.
GenTree* valx = val->gtEffectiveVal(/*commaOnly*/ true);

if (structHnd != NO_CLASS_HANDLE)
if (valStructHnd != NO_CLASS_HANDLE)
{
lvaSetStruct(tmp, structHnd, false);
lvaSetStruct(tmp, valStructHnd, false);
}
else
{
assert(valx->gtOper != GT_OBJ);
}
dest->gtFlags |= GTF_DONT_CSE;
valx->gtFlags |= GTF_DONT_CSE;
asg = impAssignStruct(dest, val, structHnd, (unsigned)CHECK_SPILL_NONE, pAfterStmt, ilOffset, block);
asg = impAssignStruct(dest, val, valStructHnd, (unsigned)CHECK_SPILL_NONE, pAfterStmt, ilOffset, block);
}
else
{
// We may have a scalar type variable assigned a struct value, e.g. a 'genReturnLocal'
// when the ABI calls for returning a struct as a primitive type.
// TODO-1stClassStructs: When we stop "lying" about the types for ABI purposes, the
// 'genReturnLocal' should be the original struct type.
assert(!varTypeIsStruct(valTyp) || typGetObjLayout(structHnd)->GetSize() == genTypeSize(varDsc));
assert(!varTypeIsStruct(valTyp) || ((valStructHnd != NO_CLASS_HANDLE) &&
(typGetObjLayout(valStructHnd)->GetSize() == genTypeSize(varDsc))));
asg = gtNewAssignNode(dest, val);
}

Expand Down Expand Up @@ -17245,28 +17273,28 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
}
else
{
GenTree* addr = tree->AsIndir()->Addr();
GenTree* addr = tree->AsIndir()->Addr();
FieldSeqNode* fieldSeq = nullptr;
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);
}
}
fieldSeq = addr->gtGetOp2()->AsIntCon()->gtFieldSeq;
}
else if (addr->OperGet() == GT_LCL_VAR)
else
{
GetZeroOffsetFieldMap()->Lookup(addr, &fieldSeq);
}
if (fieldSeq != nullptr)
{
structHnd = gtGetStructHandleIfPresent(addr);
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);
}
}
}
}
Expand All @@ -17290,6 +17318,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleIfPresent(GenTree* tree)
#endif
break;
}
// TODO-1stClassStructs: add a check that `structHnd != NO_CLASS_HANDLE`,
// nowadays it won't work because the right part of an ASG could have struct type without a handle
// (check `fgMorphBlockOperand(isBlkReqd`) and a few other cases.
}
return structHnd;
}
Expand Down
Loading

0 comments on commit 5da855d

Please sign in to comment.