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.

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

No retyping arm/arm64. #36866

Merged
merged 6 commits into from
Jun 8, 2020
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
30 changes: 30 additions & 0 deletions src/coreclr/src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -9161,6 +9161,36 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
return varTypeIsStruct(info.compRetNativeType) && (info.compRetBuffArg == BAD_VAR_NUM);
#endif // TARGET_XXX

#else // not FEATURE_MULTIREG_RET

// For this architecture there are no multireg returns
return false;

#endif // FEATURE_MULTIREG_RET
}

// Returns true if the method returns a value in more than one return register,
// it should replace/be merged with compMethodReturnsMultiRegRetType when #36868 is fixed.
bool compMethodReturnsResInMultiplyRegisters()
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if temporary, I think it would be good to 1) describe in what way it is different (in particular talk about the special handling required for SIMD types, soon to be additional ones for x64/ux, that are passed in a single register), and 2) rename it to be something like compMethodReturnsMultiRegRegTypeAlternate.

{
#if FEATURE_MULTIREG_RET
#if defined(TARGET_X86)
// On x86 only 64-bit longs are returned in multiple registers
return varTypeIsLong(info.compRetNativeType);
#else // targets: X64-UNIX, ARM64 or ARM32
sandreenko marked this conversation as resolved.
Show resolved Hide resolved
#if defined(TARGET_ARM64)
// TYP_SIMD16 is returned in one register.
if (info.compRetNativeType == TYP_SIMD16)
{
return false;
}
#endif
// On all other targets that support multireg return values:
// Methods returning a struct in multiple registers have a return value of TYP_STRUCT.
// Such method's compRetNativeType is TYP_STRUCT without a hidden RetBufArg
return varTypeIsStruct(info.compRetNativeType) && (info.compRetBuffArg == BAD_VAR_NUM);
#endif // TARGET_XXX

#else // not FEATURE_MULTIREG_RET

// For this architecture there are no multireg returns
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15149,6 +15149,11 @@ GenTree* Compiler::gtNewTempAssign(
assert(tmp == genReturnLocal);
ok = true;
}
else if (varTypeIsSIMD(dstTyp) && (valTyp == TYP_STRUCT))
{
assert(val->IsCall());
ok = true;
}

if (!ok)
{
Expand Down
8 changes: 1 addition & 7 deletions src/coreclr/src/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -4257,13 +4257,7 @@ struct GenTreeCall final : public GenTree
{
return true;
}
#elif defined(FEATURE_HFA) && defined(TARGET_ARM64)
// SIMD types are returned in vector regs on ARM64.
if (varTypeIsSIMD(gtType))
{
return false;
}
#endif // FEATURE_HFA && TARGET_ARM64
#endif

if (!varTypeIsStruct(gtType) || HasRetBufArg())
{
Expand Down
24 changes: 17 additions & 7 deletions src/coreclr/src/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -803,7 +803,7 @@ void Compiler::impAssignTempGen(unsigned tmpNum,
var_types varType = lvaTable[tmpNum].lvType;
assert(tiVerificationNeeded || varType == TYP_UNDEF || varTypeIsStruct(varType));
lvaSetStruct(tmpNum, structType, false);

varType = lvaTable[tmpNum].lvType;
// Now, set the type of the struct value. Note that lvaSetStruct may modify the type
sandreenko marked this conversation as resolved.
Show resolved Hide resolved
// of the lclVar to a specialized type (e.g. TYP_SIMD), based on the handle (structType)
// that has been passed in for the value being assigned to the temp, in which case we
Expand All @@ -812,9 +812,12 @@ void Compiler::impAssignTempGen(unsigned tmpNum,
// type, this would not be necessary - but that requires additional JIT/EE interface
// calls that may not actually be required - e.g. if we only access a field of a struct.

val->gtType = lvaTable[tmpNum].lvType;
if (compDoOldStructRetyping())
{
val->gtType = varType;
}

GenTree* dst = gtNewLclvNode(tmpNum, val->gtType);
GenTree* dst = gtNewLclvNode(tmpNum, varType);
asg = impAssignStruct(dst, val, structType, curLevel, pAfterStmt, ilOffset, block);
}
else
Expand Down Expand Up @@ -1224,7 +1227,7 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
lcl->gtFlags |= GTF_DONT_CSE;
varDsc->lvIsMultiRegRet = true;
}
else if (lcl->gtType != src->gtType)
else if ((lcl->gtType != src->gtType) && compDoOldStructRetyping())
{
// We change this to a GT_LCL_FLD (from a GT_ADDR of a GT_LCL_VAR)
lcl->ChangeOper(GT_LCL_FLD);
Expand Down Expand Up @@ -1434,7 +1437,7 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
dest = gtNewOperNode(GT_IND, asgType, destAddr);
}
}
else
else if (compDoOldStructRetyping())
{
dest->gtType = asgType;
}
Expand Down Expand Up @@ -8011,7 +8014,7 @@ var_types Compiler::impImportCall(OPCODE opcode,

CORINFO_CLASS_HANDLE actualMethodRetTypeSigClass;
actualMethodRetTypeSigClass = sig->retTypeSigClass;
if (varTypeIsStruct(callRetTyp))
if (varTypeIsStruct(callRetTyp) && compDoOldStructRetyping())
{
callRetTyp = impNormStructType(actualMethodRetTypeSigClass);
call->gtType = callRetTyp;
Expand Down Expand Up @@ -16656,7 +16659,14 @@ bool Compiler::impReturnInstruction(int prefixFlags, OPCODE& opcode)
impAssignTempGen(lvaInlineeReturnSpillTemp, op2, se.seTypeInfo.GetClassHandle(),
(unsigned)CHECK_SPILL_ALL);

GenTree* tmpOp2 = gtNewLclvNode(lvaInlineeReturnSpillTemp, op2->TypeGet());
var_types lclRetType = op2->TypeGet();
if (!compDoOldStructRetyping())
{
LclVarDsc* varDsc = lvaGetDesc(lvaInlineeReturnSpillTemp);
lclRetType = varDsc->lvType;
}

GenTree* tmpOp2 = gtNewLclvNode(lvaInlineeReturnSpillTemp, lclRetType);

if (compDoOldStructRetyping())
{
Expand Down
7 changes: 1 addition & 6 deletions src/coreclr/src/jit/jitconfigvalues.h
Original file line number Diff line number Diff line change
Expand Up @@ -437,13 +437,8 @@ CONFIG_INTEGER(JitSaveFpLrWithCalleeSavedRegisters, W("JitSaveFpLrWithCalleeSave
#endif // defined(TARGET_ARM64)
#endif // DEBUG

#if defined(TARGET_ARMARCH)
CONFIG_INTEGER(JitDoOldStructRetyping, W("JitDoOldStructRetyping"), 1) // Allow Jit to retype structs as primitive types
CONFIG_INTEGER(JitDoOldStructRetyping, W("JitDoOldStructRetyping"), 0) // Allow Jit to retype structs as primitive types
// when possible.
#else
CONFIG_INTEGER(JitDoOldStructRetyping, W("JitDoOldStructRetyping"), 1) // Allow Jit to retype structs as primitive types
// when possible.
#endif

#undef CONFIG_INTEGER
#undef CONFIG_STRING
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/jit/lclvars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1630,6 +1630,7 @@ void Compiler::StructPromotionHelper::CheckRetypedAsScalar(CORINFO_FIELD_HANDLE
//
bool Compiler::StructPromotionHelper::CanPromoteStructType(CORINFO_CLASS_HANDLE typeHnd)
{
assert(typeHnd != nullptr);
if (!compiler->eeIsValueClass(typeHnd))
{
// TODO-ObjectStackAllocation: Enable promotion of fields of stack-allocated objects.
Expand Down Expand Up @@ -1865,6 +1866,7 @@ bool Compiler::StructPromotionHelper::CanPromoteStructVar(unsigned lclNum)
}

CORINFO_CLASS_HANDLE typeHnd = varDsc->lvVerTypeInfo.GetClassHandle();
assert(typeHnd != nullptr);
return CanPromoteStructType(typeHnd);
}

Expand Down
72 changes: 55 additions & 17 deletions src/coreclr/src/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2975,9 +2975,8 @@ void Lowering::LowerRet(GenTreeUnOp* ret)
}
}
#endif // DEBUG
if (varTypeIsStruct(ret) && !comp->compMethodReturnsMultiRegRetType())
if (varTypeIsStruct(ret))
{
assert(!comp->compDoOldStructRetyping());
LowerRetStruct(ret);
}
}
Expand Down Expand Up @@ -3103,7 +3102,29 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
//
void Lowering::LowerRetStruct(GenTreeUnOp* ret)
{
assert(!comp->compMethodReturnsMultiRegRetType());
#if defined(FEATURE_HFA) && defined(TARGET_ARM64)
if (ret->TypeIs(TYP_SIMD16))
{
if (comp->info.compRetNativeType == TYP_STRUCT)
{
assert(!comp->compDoOldStructRetyping());
assert(ret->gtGetOp1()->TypeIs(TYP_SIMD16));
assert(comp->compMethodReturnsResInMultiplyRegisters());
ret->ChangeType(comp->info.compRetNativeType);
}
else
{
assert(comp->info.compRetNativeType == TYP_SIMD16);
return;
}
}
#endif

if (comp->compMethodReturnsResInMultiplyRegisters())
{
return;
}

assert(!comp->compDoOldStructRetyping());
assert(ret->OperIs(GT_RETURN));
assert(varTypeIsStruct(ret));
Expand Down Expand Up @@ -3190,7 +3211,7 @@ void Lowering::LowerRetStruct(GenTreeUnOp* ret)
//
void Lowering::LowerRetStructLclVar(GenTreeUnOp* ret)
{
assert(!comp->compMethodReturnsMultiRegRetType());
assert(!comp->compMethodReturnsResInMultiplyRegisters());
sandreenko marked this conversation as resolved.
Show resolved Hide resolved
assert(!comp->compDoOldStructRetyping());
assert(ret->OperIs(GT_RETURN));
GenTreeLclVarCommon* lclVar = ret->gtGetOp1()->AsLclVar();
Expand Down Expand Up @@ -3271,18 +3292,31 @@ void Lowering::LowerCallStruct(GenTreeCall* call)
return;
}

#ifdef TARGET_ARMARCH
// !compDoOldStructRetyping is not supported on arm yet,
// because of HFA.
assert(comp->compDoOldStructRetyping());
return;
#else // !TARGET_ARMARCH
#if defined(FEATURE_HFA)
if (comp->IsHfa(call))
{
#if defined(TARGET_ARM64)
assert(comp->GetHfaCount(call) == 1);
#elif defined(TARGET_ARM)
// ARM returns double in 2 float registers, but
// `call->HasMultiRegRetVal()` count double registers.
assert(comp->GetHfaCount(call) <= 2);
#elif // !TARGET_ARM64 && !TARGET_ARM
unreached();
#endif // !TARGET_ARM64 && !TARGET_ARM
var_types hfaType = comp->GetHfaType(call);
if (call->TypeIs(hfaType))
{
return;
}
}
#endif // FEATURE_HFA

assert(!comp->compDoOldStructRetyping());
CORINFO_CLASS_HANDLE retClsHnd = call->gtRetClsHnd;
Compiler::structPassingKind howToReturnStruct;
var_types returnType = comp->getReturnTypeForStruct(retClsHnd, &howToReturnStruct);
assert(!varTypeIsStruct(returnType) && returnType != TYP_UNKNOWN);
assert(returnType != TYP_STRUCT && returnType != TYP_UNKNOWN);
var_types origType = call->TypeGet();
call->gtType = genActualType(returnType);

Expand All @@ -3297,12 +3331,18 @@ void Lowering::LowerCallStruct(GenTreeCall* call)
case GT_STORE_BLK:
case GT_STORE_OBJ:
// Leave as is, the user will handle it.
assert(user->TypeIs(origType));
assert(user->TypeIs(origType) || varTypeIsSIMD(user->TypeGet()));
break;

#ifdef FEATURE_SIMD
case GT_STORE_LCL_FLD:
assert(varTypeIsSIMD(user) && (returnType == user->TypeGet()));
break;
#endif // FEATURE_SIMD

case GT_STOREIND:
#ifdef FEATURE_SIMD
if (user->TypeIs(TYP_SIMD8))
if (varTypeIsSIMD(user))
{
user->ChangeType(returnType);
break;
Expand All @@ -3319,7 +3359,6 @@ void Lowering::LowerCallStruct(GenTreeCall* call)
unreached();
}
}
#endif // !TARGET_ARMARCH
}

//----------------------------------------------------------------------------------------------
Expand All @@ -3338,9 +3377,8 @@ void Lowering::LowerStoreCallStruct(GenTreeBlk* store)
assert(store->Data()->IsCall());
GenTreeCall* call = store->Data()->AsCall();

const ClassLayout* layout = store->GetLayout();
assert(layout->GetSlotCount() == 1);
const var_types regType = layout->GetRegisterType();
const ClassLayout* layout = store->GetLayout();
const var_types regType = layout->GetRegisterType();

unsigned storeSize = store->GetLayout()->GetSize();
if (regType != TYP_UNDEF)
Expand Down
17 changes: 14 additions & 3 deletions src/coreclr/src/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10191,8 +10191,19 @@ GenTree* Compiler::fgMorphBlockOperand(GenTree* tree, var_types asgType, unsigne
}
else if (effectiveVal->TypeGet() != asgType)
{
GenTree* addr = gtNewOperNode(GT_ADDR, TYP_BYREF, effectiveVal);
effectiveVal = gtNewIndir(asgType, addr);
if (effectiveVal->IsCall())
{
#ifdef DEBUG
GenTreeCall* call = effectiveVal->AsCall();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this block will go away after #11413 is fixed as discussed in #37341

assert(call->TypeGet() == TYP_STRUCT);
assert(blockWidth == info.compCompHnd->getClassSize(call->gtRetClsHnd));
#endif
}
else
{
GenTree* addr = gtNewOperNode(GT_ADDR, TYP_BYREF, effectiveVal);
effectiveVal = gtNewIndir(asgType, addr);
}
}
}
else
Expand Down Expand Up @@ -11941,7 +11952,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)

return tree;
}
if (tree->TypeIs(TYP_STRUCT) && op1->OperIs(GT_OBJ, GT_BLK))
if (varTypeIsStruct(tree) && op1->OperIs(GT_OBJ, GT_BLK))
{
assert(!compDoOldStructRetyping());
GenTree* addr = op1->AsBlk()->Addr();
Expand Down
Loading