From 6d58642cd13e639501fcb3c1289af3ac9cc40c34 Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Wed, 29 Aug 2018 14:58:10 -0700 Subject: [PATCH] Arm: Correctly handle multi-reg COPY Fix #19448 --- src/jit/codegenarmarch.cpp | 107 ++++++++++++++++++++++++++------------------- src/jit/gentree.h | 13 ++---- src/jit/lower.cpp | 14 +++++- src/jit/lsraarmarch.cpp | 4 +- src/jit/lsrabuild.cpp | 10 +---- 5 files changed, 84 insertions(+), 64 deletions(-) diff --git a/src/jit/codegenarmarch.cpp b/src/jit/codegenarmarch.cpp index 746636a..e59bf35 100644 --- a/src/jit/codegenarmarch.cpp +++ b/src/jit/codegenarmarch.cpp @@ -2059,56 +2059,30 @@ void CodeGen::genCodeForStoreOffset(instruction ins, emitAttr size, regNumber sr } //------------------------------------------------------------------------ -// genRegCopy: Generate a register copy. +// genRegCopy: Produce code for a GT_COPY node. +// +// Arguments: +// tree - the GT_COPY node +// +// Notes: +// This will copy the register(s) produced by this node's source, to +// the register(s) allocated to this GT_COPY node. +// It has some special handling for these cases: +// - when the source and target registers are in different register files +// (note that this is *not* a conversion). +// - when the source is a lclVar whose home location is being moved to a new +// register (rather than just being copied for temporary use). // void CodeGen::genRegCopy(GenTree* treeNode) { assert(treeNode->OperGet() == GT_COPY); + GenTree* op1 = treeNode->gtOp.gtOp1; - var_types targetType = treeNode->TypeGet(); - regNumber targetReg = treeNode->gtRegNum; - assert(targetReg != REG_NA); - - GenTree* op1 = treeNode->gtOp.gtOp1; regNumber sourceReg = genConsumeReg(op1); - // Check whether this node and the node from which we're copying the value have the same - // register type. - // This can happen if (currently iff) we have a SIMD vector type that fits in an integer - // register, in which case it is passed as an argument, or returned from a call, - // in an integer register and must be copied if it's in an xmm register. - - if (varTypeIsFloating(treeNode) != varTypeIsFloating(op1)) - { -#ifdef _TARGET_ARM64_ - inst_RV_RV(INS_fmov, targetReg, sourceReg, targetType); -#else // !_TARGET_ARM64_ - if (varTypeIsFloating(treeNode)) - { - // GT_COPY from 'int' to 'float' currently can't happen. Maybe if ARM SIMD is implemented - // it will happen, according to the comment above? - NYI_ARM("genRegCopy from 'int' to 'float'"); - } - else - { - assert(varTypeIsFloating(op1)); - - if (op1->TypeGet() == TYP_FLOAT) - { - inst_RV_RV(INS_vmov_f2i, targetReg, genConsumeReg(op1), targetType); - } - else - { - regNumber otherReg = (regNumber)treeNode->AsCopyOrReload()->gtOtherRegs[0]; - assert(otherReg != REG_NA); - inst_RV_RV_RV(INS_vmov_d2i, targetReg, otherReg, genConsumeReg(op1), EA_8BYTE); - } - } -#endif // !_TARGET_ARM64_ - } - else if (targetType == TYP_STRUCT) + if (op1->IsMultiRegNode()) { - noway_assert(op1->IsMultiRegNode() && !op1->IsCopyOrReload()); + noway_assert(!op1->IsCopyOrReload()); unsigned regCount = op1->GetMultiRegCount(); for (unsigned i = 0; i < regCount; i++) { @@ -2120,7 +2094,51 @@ void CodeGen::genRegCopy(GenTree* treeNode) } else { - inst_RV_RV(ins_Copy(targetType), targetReg, sourceReg, targetType); + var_types targetType = treeNode->TypeGet(); + regNumber targetReg = treeNode->gtRegNum; + assert(targetReg != REG_NA); + assert(targetType != TYP_STRUCT); + + // Check whether this node and the node from which we're copying the value have the same + // register type. + // This can happen if (currently iff) we have a SIMD vector type that fits in an integer + // register, in which case it is passed as an argument, or returned from a call, + // in an integer register and must be copied if it's in a floating point register. + + bool srcFltReg = (varTypeIsFloating(op1) || varTypeIsSIMD(op1)); + bool tgtFltReg = (varTypeIsFloating(treeNode) || varTypeIsSIMD(treeNode)); + if (srcFltReg != tgtFltReg) + { +#ifdef _TARGET_ARM64_ + inst_RV_RV(INS_fmov, targetReg, sourceReg, targetType); +#else // !_TARGET_ARM64_ + if (varTypeIsFloating(treeNode)) + { + // GT_COPY from 'int' to 'float' currently can't happen. Maybe if ARM SIMD is implemented + // it will happen, according to the comment above? + NYI_ARM("genRegCopy from 'int' to 'float'"); + } + else + { + assert(varTypeIsFloating(op1)); + + if (op1->TypeGet() == TYP_FLOAT) + { + inst_RV_RV(INS_vmov_f2i, targetReg, genConsumeReg(op1), targetType); + } + else + { + regNumber otherReg = (regNumber)treeNode->AsCopyOrReload()->gtOtherRegs[0]; + assert(otherReg != REG_NA); + inst_RV_RV_RV(INS_vmov_d2i, targetReg, otherReg, genConsumeReg(op1), EA_8BYTE); + } + } +#endif // !_TARGET_ARM64_ + } + else + { + inst_RV_RV(ins_Copy(targetType), targetReg, sourceReg, targetType); + } } if (op1->IsLocal()) @@ -2258,7 +2276,8 @@ void CodeGen::genCallInstruction(GenTreeCall* call) #endif // _TARGET_* } - // Either gtControlExpr != null or gtCallAddr != null or it is a direct non-virtual call to a user or helper method. + // Either gtControlExpr != null or gtCallAddr != null or it is a direct non-virtual call to a user or helper + // method. CORINFO_METHOD_HANDLE methHnd; GenTree* target = call->gtControlExpr; if (callType == CT_INDIRECT) diff --git a/src/jit/gentree.h b/src/jit/gentree.h index d14ad1a..c646650 100644 --- a/src/jit/gentree.h +++ b/src/jit/gentree.h @@ -3618,10 +3618,6 @@ struct GenTreeCall final : public GenTree // Return Value: // True if the call is returning a multi-reg return value. False otherwise. // - // Note: - // This is implemented only for x64 Unix and yet to be implemented for - // other multi-reg return target arch (arm64/arm32/x86). - // bool HasMultiRegRetVal() const { #if defined(_TARGET_X86_) @@ -3967,14 +3963,13 @@ struct GenTreeMultiRegOp : public GenTreeOp var_types GetRegType(unsigned index) { assert(index < 2); - // The type of register is usually the same as GenTree type - // since most of time GenTreeMultiRegOp uses only a single reg (when gtOtherReg is REG_NA). - // The special case is when we have TYP_LONG here, which was `TYP_DOUBLE` originally - // (copied to int regs for argument push on armel). Then we need to separate them into int for each index. + // The type of register is usually the same as GenTree type, since GenTreeMultiRegOp usually defines a single + // reg. + // The special case is when we have TYP_LONG, which may be a MUL_LONG, or a DOUBLE arg passed as LONG, + // in which case we need to separate them into int for each index. var_types result = TypeGet(); if (result == TYP_LONG) { - assert(gtOtherReg != REG_NA); result = TYP_INT; } return result; diff --git a/src/jit/lower.cpp b/src/jit/lower.cpp index c6648b8..86005ad 100644 --- a/src/jit/lower.cpp +++ b/src/jit/lower.cpp @@ -1078,7 +1078,12 @@ GenTree* Lowering::NewPutArg(GenTreeCall* call, GenTree* arg, fgArgTabEntry* inf // Set type of registers for (unsigned index = 0; index < info->numRegs; index++) { - var_types regType = comp->getJitGCType(gcLayout[index]); + var_types regType = comp->getJitGCType(gcLayout[index]); + // Account for the possibility that float fields may be passed in integer registers. + if (varTypeIsFloating(regType) && !genIsValidFloatReg(argSplit->GetRegNumByIdx(index))) + { + regType = (regType == TYP_FLOAT) ? TYP_INT : TYP_LONG; + } argSplit->m_regType[index] = regType; } } @@ -1087,7 +1092,12 @@ GenTree* Lowering::NewPutArg(GenTreeCall* call, GenTree* arg, fgArgTabEntry* inf GenTreeFieldList* fieldListPtr = arg->AsFieldList(); for (unsigned index = 0; index < info->numRegs; fieldListPtr = fieldListPtr->Rest(), index++) { - var_types regType = fieldListPtr->gtGetOp1()->TypeGet(); + var_types regType = fieldListPtr->gtGetOp1()->TypeGet(); + // Account for the possibility that float fields may be passed in integer registers. + if (varTypeIsFloating(regType) && !genIsValidFloatReg(argSplit->GetRegNumByIdx(index))) + { + regType = (regType == TYP_FLOAT) ? TYP_INT : TYP_LONG; + } argSplit->m_regType[index] = regType; // Clear the register assignments on the fieldList nodes, as these are contained. diff --git a/src/jit/lsraarmarch.cpp b/src/jit/lsraarmarch.cpp index aee5698..611d858 100644 --- a/src/jit/lsraarmarch.cpp +++ b/src/jit/lsraarmarch.cpp @@ -480,7 +480,9 @@ int LinearScan::BuildPutArgSplit(GenTreePutArgSplit* argNode) regMaskTP argMask = RBM_NONE; for (unsigned i = 0; i < argNode->gtNumRegs; i++) { - argMask |= genRegMask((regNumber)((unsigned)argReg + i)); + regNumber thisArgReg = (regNumber)((unsigned)argReg + i); + argMask |= genRegMask(thisArgReg); + argNode->SetRegNumByIdx(thisArgReg, i); } if (putArgChild->OperGet() == GT_FIELD_LIST) diff --git a/src/jit/lsrabuild.cpp b/src/jit/lsrabuild.cpp index 8e97492..69785a7 100644 --- a/src/jit/lsrabuild.cpp +++ b/src/jit/lsrabuild.cpp @@ -2368,16 +2368,10 @@ RefPosition* LinearScan::BuildDef(GenTree* tree, regMaskTP dstCandidates, int mu assert((tree->gtRegNum == REG_NA) || (dstCandidates == genRegMask(tree->GetRegByIndex(multiRegIdx)))); } -#ifdef FEATURE_MULTIREG_ARGS_OR_RET - if (tree->TypeGet() == TYP_STRUCT) + if (tree->IsMultiRegNode()) { - // We require a fixed set of candidates for this case. - assert(isSingleRegister(dstCandidates)); - type = (dstCandidates & allRegs(TYP_FLOAT)) != RBM_NONE ? TYP_FLOAT : TYP_INT; + type = tree->GetRegTypeByIndex(multiRegIdx); } -#else - assert(!tree->TypeGet() == TYP_STRUCT); -#endif Interval* interval = newInterval(type); if (tree->gtRegNum != REG_NA) -- 2.7.4