From bb7356e1bd40e3f274153a3bc12a85bb6b04c293 Mon Sep 17 00:00:00 2001 From: Sergey Date: Tue, 4 May 2021 16:36:17 -0700 Subject: [PATCH 1/2] Support STORE_LCL_VAR(0) after lowering. --- src/coreclr/jit/codegenarm64.cpp | 17 ++------- src/coreclr/jit/gentree.h | 30 ++++++++-------- src/coreclr/jit/lower.cpp | 53 +++++++++++++++++++++++++++- src/coreclr/jit/rationalize.cpp | 24 +++++++------ src/coreclr/jit/simdcodegenxarch.cpp | 6 +--- 5 files changed, 84 insertions(+), 46 deletions(-) diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index d306c3d438581..d20e538a397c3 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -3846,20 +3846,9 @@ void CodeGen::genEmitHelperCall(unsigned helper, int argSize, emitAttr retSize, void CodeGen::genSIMDIntrinsic(GenTreeSIMD* simdNode) { // NYI for unsupported base types - if (simdNode->GetSimdBaseType() != TYP_INT && simdNode->GetSimdBaseType() != TYP_LONG && - simdNode->GetSimdBaseType() != TYP_FLOAT && simdNode->GetSimdBaseType() != TYP_DOUBLE && - simdNode->GetSimdBaseType() != TYP_USHORT && simdNode->GetSimdBaseType() != TYP_UBYTE && - simdNode->GetSimdBaseType() != TYP_SHORT && simdNode->GetSimdBaseType() != TYP_BYTE && - simdNode->GetSimdBaseType() != TYP_UINT && simdNode->GetSimdBaseType() != TYP_ULONG) - { - // We don't need a base type for the Upper Save & Restore intrinsics, and we may find - // these implemented over lclVars created by CSE without full handle information (and - // therefore potentially without a base type). - if ((simdNode->gtSIMDIntrinsicID != SIMDIntrinsicUpperSave) && - (simdNode->gtSIMDIntrinsicID != SIMDIntrinsicUpperRestore)) - { - noway_assert(!"SIMD intrinsic with unsupported base type."); - } + if (!varTypeIsArithmetic(simdNode->GetSimdBaseType())) + { + noway_assert(!"SIMD intrinsic with unsupported base type."); } switch (simdNode->gtSIMDIntrinsicID) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index c751c7f4c7df6..0946c8881ccbc 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -1688,10 +1688,10 @@ struct GenTree bool IsValidCallArgument(); #endif // DEBUG - inline bool IsFPZero(); - inline bool IsIntegralConst(ssize_t constVal); - inline bool IsIntegralConstVector(ssize_t constVal); - inline bool IsSIMDZero(); + inline bool IsFPZero() const; + inline bool IsIntegralConst(ssize_t constVal) const; + inline bool IsIntegralConstVector(ssize_t constVal) const; + inline bool IsSIMDZero() const; inline bool IsBoxedValue(); @@ -2922,11 +2922,11 @@ struct GenTreeVal : public GenTree struct GenTreeIntConCommon : public GenTree { - inline INT64 LngValue(); + inline INT64 LngValue() const; inline void SetLngValue(INT64 val); - inline ssize_t IconValue(); + inline ssize_t IconValue() const; inline void SetIconValue(ssize_t val); - inline INT64 IntegralValue(); + inline INT64 IntegralValue() const; GenTreeIntConCommon(genTreeOps oper, var_types type DEBUGARG(bool largeNode = false)) : GenTree(oper, type DEBUGARG(largeNode)) @@ -3090,7 +3090,7 @@ struct GenTreeLngCon : public GenTreeIntConCommon #endif }; -inline INT64 GenTreeIntConCommon::LngValue() +inline INT64 GenTreeIntConCommon::LngValue() const { #ifndef TARGET_64BIT assert(gtOper == GT_CNS_LNG); @@ -3114,7 +3114,7 @@ inline void GenTreeIntConCommon::SetLngValue(INT64 val) #endif } -inline ssize_t GenTreeIntConCommon::IconValue() +inline ssize_t GenTreeIntConCommon::IconValue() const { assert(gtOper == GT_CNS_INT); // We should never see a GT_CNS_LNG for a 64-bit target! return AsIntCon()->gtIconVal; @@ -3126,7 +3126,7 @@ inline void GenTreeIntConCommon::SetIconValue(ssize_t val) AsIntCon()->gtIconVal = val; } -inline INT64 GenTreeIntConCommon::IntegralValue() +inline INT64 GenTreeIntConCommon::IntegralValue() const { #ifdef TARGET_64BIT return LngValue(); @@ -6942,7 +6942,7 @@ inline bool GenTree::OperIsCopyBlkOp() // Return Value: // Returns true iff the tree is an GT_CNS_DBL, with value of 0.0. -inline bool GenTree::IsFPZero() +inline bool GenTree::IsFPZero() const { if ((gtOper == GT_CNS_DBL) && (AsDblCon()->gtDconVal == 0.0)) { @@ -6965,7 +6965,7 @@ inline bool GenTree::IsFPZero() // Like gtIconVal, the argument is of ssize_t, so cannot check for // long constants in a target-independent way. -inline bool GenTree::IsIntegralConst(ssize_t constVal) +inline bool GenTree::IsIntegralConst(ssize_t constVal) const { if ((gtOper == GT_CNS_INT) && (AsIntConCommon()->IconValue() == constVal)) @@ -6991,7 +6991,7 @@ inline bool GenTree::IsIntegralConst(ssize_t constVal) // Returns: // True if this represents an integral const SIMD vector. // -inline bool GenTree::IsIntegralConstVector(ssize_t constVal) +inline bool GenTree::IsIntegralConstVector(ssize_t constVal) const { #ifdef FEATURE_SIMD // SIMDIntrinsicInit intrinsic with a const value as initializer @@ -7008,7 +7008,7 @@ inline bool GenTree::IsIntegralConstVector(ssize_t constVal) #ifdef FEATURE_HW_INTRINSICS if (gtOper == GT_HWINTRINSIC) { - GenTreeHWIntrinsic* node = AsHWIntrinsic(); + const GenTreeHWIntrinsic* node = AsHWIntrinsic(); if (!varTypeIsIntegral(node->GetSimdBaseType())) { @@ -7058,7 +7058,7 @@ inline bool GenTree::IsIntegralConstVector(ssize_t constVal) // Returns: // True if this represents an integral const SIMD vector. // -inline bool GenTree::IsSIMDZero() +inline bool GenTree::IsSIMDZero() const { #ifdef FEATURE_SIMD if ((gtOper == GT_SIMD) && (AsSIMD()->gtSIMDIntrinsicID == SIMDIntrinsicInit)) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index f72943b7f95e1..052ce6fa372dd 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3120,6 +3120,7 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore) } if ((lclStore->TypeGet() == TYP_STRUCT) && !srcIsMultiReg && (src->OperGet() != GT_PHI)) { + bool convertToStoreObj; if (src->OperGet() == GT_CALL) { GenTreeCall* call = src->AsCall(); @@ -3165,8 +3166,58 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore) return; } #endif // !WINDOWS_AMD64_ABI + convertToStoreObj = false; } - else if (!src->OperIs(GT_LCL_VAR) || (varDsc->GetLayout()->GetRegisterType() == TYP_UNDEF)) + else if (!varDsc->IsEnregisterable()) + { + convertToStoreObj = true; + } + else if (src->OperIs(GT_CNS_INT)) + { + assert(src->IsIntegralConst(0) && "expected an INIT_VAL for non-zero init."); + var_types regType = varDsc->GetRegisterType(); +#ifdef FEATURE_SIMD + if (varTypeIsSIMD(regType)) + { + if (!varDsc->lvDoNotEnregister) + { + CorInfoType simdBaseJitType = comp->getBaseJitTypeOfSIMDLocal(lclStore); + if (simdBaseJitType == CORINFO_TYPE_UNDEF) + { + // Lie about the type if we don't know/have it. + simdBaseJitType = CORINFO_TYPE_FLOAT; + } + GenTreeSIMD* simdTree = + comp->gtNewSIMDNode(regType, src, SIMDIntrinsicInit, simdBaseJitType, varDsc->lvExactSize); + BlockRange().InsertAfter(src, simdTree); + src = simdTree; + lclStore->gtOp1 = src; + convertToStoreObj = false; + } + else + { + // the local is already known to be on stack, cheaper to initialize 0 there then use simd regs. + // TODO-CQ: keep STORE_LCL and convert to non-simd after LSRA. + convertToStoreObj = true; + } + } + else +#endif // FEATURE_SIMD + { + convertToStoreObj = false; + } + } + else if (!src->OperIs(GT_LCL_VAR)) + { + convertToStoreObj = true; + } + else + { + assert(src->OperIs(GT_LCL_VAR)); + convertToStoreObj = false; + } + + if (convertToStoreObj) { GenTreeLclVar* addr = comp->gtNewLclVarAddrNode(lclStore->GetLclNum(), TYP_BYREF); diff --git a/src/coreclr/jit/rationalize.cpp b/src/coreclr/jit/rationalize.cpp index e15bc54d3afab..52f1cd4a69a55 100644 --- a/src/coreclr/jit/rationalize.cpp +++ b/src/coreclr/jit/rationalize.cpp @@ -398,23 +398,25 @@ void Rationalizer::RewriteAssignment(LIR::Use& use) #ifdef FEATURE_SIMD if (varTypeIsSIMD(location) && assignment->OperIsInitBlkOp()) { - if (location->OperGet() == GT_LCL_VAR) + if (location->OperIs(GT_LCL_VAR)) { var_types simdType = location->TypeGet(); GenTree* initVal = assignment->AsOp()->gtOp2; CorInfoType simdBaseJitType = comp->getBaseJitTypeOfSIMDLocal(location); - if (simdBaseJitType != CORINFO_TYPE_UNDEF) + if (simdBaseJitType == CORINFO_TYPE_UNDEF) { - GenTreeSIMD* simdTree = new (comp, GT_SIMD) - GenTreeSIMD(simdType, initVal, SIMDIntrinsicInit, simdBaseJitType, genTypeSize(simdType)); - assignment->AsOp()->gtOp2 = simdTree; - value = simdTree; - initVal->gtNext = simdTree; - simdTree->gtPrev = initVal; - - simdTree->gtNext = location; - location->gtPrev = simdTree; + // Lie about the type if we don't know/have it. + simdBaseJitType = CORINFO_TYPE_FLOAT; } + GenTreeSIMD* simdTree = + comp->gtNewSIMDNode(simdType, initVal, SIMDIntrinsicInit, simdBaseJitType, genTypeSize(simdType)); + assignment->AsOp()->gtOp2 = simdTree; + value = simdTree; + initVal->gtNext = simdTree; + simdTree->gtPrev = initVal; + + simdTree->gtNext = location; + location->gtPrev = simdTree; } } #endif // FEATURE_SIMD diff --git a/src/coreclr/jit/simdcodegenxarch.cpp b/src/coreclr/jit/simdcodegenxarch.cpp index 13c36742e744d..0ceac8c6c26fd 100644 --- a/src/coreclr/jit/simdcodegenxarch.cpp +++ b/src/coreclr/jit/simdcodegenxarch.cpp @@ -2353,11 +2353,7 @@ void CodeGen::genSIMDIntrinsicUpperRestore(GenTreeSIMD* simdNode) void CodeGen::genSIMDIntrinsic(GenTreeSIMD* simdNode) { // NYI for unsupported base types - if (simdNode->GetSimdBaseType() != TYP_INT && simdNode->GetSimdBaseType() != TYP_LONG && - simdNode->GetSimdBaseType() != TYP_FLOAT && simdNode->GetSimdBaseType() != TYP_DOUBLE && - simdNode->GetSimdBaseType() != TYP_USHORT && simdNode->GetSimdBaseType() != TYP_UBYTE && - simdNode->GetSimdBaseType() != TYP_SHORT && simdNode->GetSimdBaseType() != TYP_BYTE && - simdNode->GetSimdBaseType() != TYP_UINT && simdNode->GetSimdBaseType() != TYP_ULONG) + if (!varTypeIsArithmetic(simdNode->GetSimdBaseType())) { noway_assert(!"SIMD intrinsic with unsupported base type."); } From e8e2a8b19209e4599dd7c735e2c52c8f7139bba5 Mon Sep 17 00:00:00 2001 From: Sergey Date: Fri, 7 May 2021 10:49:31 -0700 Subject: [PATCH 2/2] fix a nice catch from Tanner --- src/coreclr/jit/lower.cpp | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 052ce6fa372dd..1a48600722674 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3179,27 +3179,19 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore) #ifdef FEATURE_SIMD if (varTypeIsSIMD(regType)) { - if (!varDsc->lvDoNotEnregister) + CorInfoType simdBaseJitType = comp->getBaseJitTypeOfSIMDLocal(lclStore); + if (simdBaseJitType == CORINFO_TYPE_UNDEF) { - CorInfoType simdBaseJitType = comp->getBaseJitTypeOfSIMDLocal(lclStore); - if (simdBaseJitType == CORINFO_TYPE_UNDEF) - { - // Lie about the type if we don't know/have it. - simdBaseJitType = CORINFO_TYPE_FLOAT; - } - GenTreeSIMD* simdTree = - comp->gtNewSIMDNode(regType, src, SIMDIntrinsicInit, simdBaseJitType, varDsc->lvExactSize); - BlockRange().InsertAfter(src, simdTree); - src = simdTree; - lclStore->gtOp1 = src; - convertToStoreObj = false; - } - else - { - // the local is already known to be on stack, cheaper to initialize 0 there then use simd regs. - // TODO-CQ: keep STORE_LCL and convert to non-simd after LSRA. - convertToStoreObj = true; + // Lie about the type if we don't know/have it. + simdBaseJitType = CORINFO_TYPE_FLOAT; } + GenTreeSIMD* simdTree = + comp->gtNewSIMDNode(regType, src, SIMDIntrinsicInit, simdBaseJitType, varDsc->lvExactSize); + BlockRange().InsertAfter(src, simdTree); + LowerSIMD(simdTree); + src = simdTree; + lclStore->gtOp1 = src; + convertToStoreObj = false; } else #endif // FEATURE_SIMD