From 0261ea7c2f60fa7969cac439954ca84e798b7c86 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 7 Oct 2019 04:23:57 +0300 Subject: [PATCH 01/20] improve Math(F).FusedMultiplyAdd --- src/jit/importer.cpp | 43 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 2a9faf4d816a..0befc9a3c90e 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -4029,14 +4029,49 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // Vector128.CreateScalar(z) // ).ToScalar(); - GenTree* op3 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, impPopStack().val, + // Depending on additional GT_NEG nodes it can be: + // + // X * Y + Z -> NI_FMA_MultiplyAddScalar + // X * -Y + Z -> NI_FMA_MultiplyAddNegatedScalar + // -X * Y + Z -> NI_FMA_MultiplyAddNegatedScalar + // -X * -Y + Z -> NI_FMA_MultiplyAddScalar + // X * Y - Z -> NI_FMA_MultiplySubtractScalar + // X * -Y - Z -> NI_FMA_MultiplySubtractNegatedScalar + // -X * Y - Z -> NI_FMA_MultiplySubtractNegatedScalar + // -X * -Y - Z -> NI_FMA_MultiplySubtractScalar + + GenTree* a = impPopStack().val; + GenTree* b = impPopStack().val; + GenTree* c = impPopStack().val; + + bool negated = a->OperIs(GT_NEG) ^ b->OperIs(GT_NEG); + NamedIntrinsic fma; + if (a->OperIs(GT_NEG)) + { + a = a->gtGetOp1(); + } + if (b->OperIs(GT_NEG)) + { + b = b->gtGetOp1(); + } + if (c->OperIs(GT_NEG)) + { + c = c->gtGetOp1(); + fma = negated ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; + } + else + { + fma = negated ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; + } + + GenTree* op3 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, a, NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op2 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, impPopStack().val, + GenTree* op2 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, b, NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op1 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, impPopStack().val, + GenTree* op1 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, c, NI_Vector128_CreateScalarUnsafe, callType, 16); GenTree* res = - gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, NI_FMA_MultiplyAddScalar, callType, 16); + gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, fma, callType, 16); retNode = gtNewSimdHWIntrinsicNode(callType, res, NI_Vector128_ToScalar, callType, 16); } From 029628622bf7a19372f7f94429e1b7fbaa48952e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 7 Oct 2019 13:21:32 +0300 Subject: [PATCH 02/20] fix wrong order and clang formatting --- src/jit/importer.cpp | 59 ++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 0befc9a3c90e..2404c67bb63b 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -4031,47 +4031,46 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // Depending on additional GT_NEG nodes it can be: // - // X * Y + Z -> NI_FMA_MultiplyAddScalar - // X * -Y + Z -> NI_FMA_MultiplyAddNegatedScalar - // -X * Y + Z -> NI_FMA_MultiplyAddNegatedScalar - // -X * -Y + Z -> NI_FMA_MultiplyAddScalar - // X * Y - Z -> NI_FMA_MultiplySubtractScalar - // X * -Y - Z -> NI_FMA_MultiplySubtractNegatedScalar - // -X * Y - Z -> NI_FMA_MultiplySubtractNegatedScalar - // -X * -Y - Z -> NI_FMA_MultiplySubtractScalar - - GenTree* a = impPopStack().val; - GenTree* b = impPopStack().val; - GenTree* c = impPopStack().val; - - bool negated = a->OperIs(GT_NEG) ^ b->OperIs(GT_NEG); + // x * y + z -> NI_FMA_MultiplyAddScalar + // x * -y + z -> NI_FMA_MultiplyAddNegatedScalar + // -x * y + z -> NI_FMA_MultiplyAddNegatedScalar + // -x * -y + z -> NI_FMA_MultiplyAddScalar + // x * y - z -> NI_FMA_MultiplySubtractScalar + // x * -y - z -> NI_FMA_MultiplySubtractNegatedScalar + // -x * y - z -> NI_FMA_MultiplySubtractNegatedScalar + // -x * -y - z -> NI_FMA_MultiplySubtractScalar + + GenTree* x = impPopStack().val; + GenTree* y = impPopStack().val; + GenTree* z = impPopStack().val; + + bool negMul = x->OperIs(GT_NEG) ^ y->OperIs(GT_NEG); NamedIntrinsic fma; - if (a->OperIs(GT_NEG)) + if (x->OperIs(GT_NEG)) { - a = a->gtGetOp1(); + x = x->gtGetOp1(); } - if (b->OperIs(GT_NEG)) + if (y->OperIs(GT_NEG)) { - b = b->gtGetOp1(); + y = y->gtGetOp1(); } - if (c->OperIs(GT_NEG)) + if (z->OperIs(GT_NEG)) { - c = c->gtGetOp1(); - fma = negated ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; + z = z->gtGetOp1(); + fma = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else { - fma = negated ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; + fma = negMul ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; } - GenTree* op3 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, a, - NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op2 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, b, - NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op1 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, c, - NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* res = - gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, fma, callType, 16); + GenTree* op3 = + gtNewSimdHWIntrinsicNode(TYP_SIMD16, z, NI_Vector128_CreateScalarUnsafe, callType, 16); + GenTree* op2 = + gtNewSimdHWIntrinsicNode(TYP_SIMD16, y, NI_Vector128_CreateScalarUnsafe, callType, 16); + GenTree* op1 = + gtNewSimdHWIntrinsicNode(TYP_SIMD16, x, NI_Vector128_CreateScalarUnsafe, callType, 16); + GenTree* res = gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, fma, callType, 16); retNode = gtNewSimdHWIntrinsicNode(callType, res, NI_Vector128_ToScalar, callType, 16); } From a23c646f82163682d15b79b73269f02b59f249fe Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 7 Oct 2019 13:25:42 +0300 Subject: [PATCH 03/20] fix ordering again --- src/jit/importer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 2404c67bb63b..1bede87f0f50 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -4040,9 +4040,9 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // -x * y - z -> NI_FMA_MultiplySubtractNegatedScalar // -x * -y - z -> NI_FMA_MultiplySubtractScalar - GenTree* x = impPopStack().val; - GenTree* y = impPopStack().val; GenTree* z = impPopStack().val; + GenTree* y = impPopStack().val; + GenTree* x = impPopStack().val; bool negMul = x->OperIs(GT_NEG) ^ y->OperIs(GT_NEG); NamedIntrinsic fma; From 6ebd6361e31faf3ce6bcb0b3dd9d316685c92033 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 7 Oct 2019 13:40:58 +0300 Subject: [PATCH 04/20] code formatting --- src/jit/importer.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 1bede87f0f50..9fc38db85487 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -4056,7 +4056,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, } if (z->OperIs(GT_NEG)) { - z = z->gtGetOp1(); + z = z->gtGetOp1(); fma = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else @@ -4064,11 +4064,11 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, fma = negMul ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; } - GenTree* op3 = + GenTree* op3 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, z, NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op2 = + GenTree* op2 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, y, NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op1 = + GenTree* op1 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, x, NI_Vector128_CreateScalarUnsafe, callType, 16); GenTree* res = gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, fma, callType, 16); From 60a9df40a4581e68bc109d1a085f4544aa93871f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 7 Oct 2019 18:11:34 +0300 Subject: [PATCH 05/20] fix typos --- src/jit/instrsxarch.h | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/jit/instrsxarch.h b/src/jit/instrsxarch.h index 0b98f4973ae4..33ece4497deb 100644 --- a/src/jit/instrsxarch.h +++ b/src/jit/instrsxarch.h @@ -557,30 +557,30 @@ INST3(vfmsub231sd, "fmsub231sd", IUM_WR, BAD_CODE, BAD_CODE, INST3(vfmsub132ss, "fmsub132ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9B), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Multiply-Subtract of Scalar Single-Precision Floating-Point Values INST3(vfmsub213ss, "fmsub213ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAB), INS_Flags_IsDstDstSrcAVXInstruction) // INST3(vfmsub231ss, "fmsub231ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBB), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd132pd, "fmnadd132pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9C), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Packed Double-Precision Floating-Point Values -INST3(vfnmadd213pd, "fmnadd213pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAC), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd231pd, "fmnadd231pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBC), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd132ps, "fmnadd132ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9C), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Packed Single-Precision Floating-Point Values -INST3(vfnmadd213ps, "fmnadd213ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAC), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd231ps, "fmnadd231ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBC), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd132sd, "fmnadd132sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9D), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Scalar Double-Precision Floating-Point Values -INST3(vfnmadd213sd, "fmnadd213sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAD), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd231sd, "fmnadd231sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBD), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd132ss, "fmnadd132ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9D), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Scalar Single-Precision Floating-Point Values -INST3(vfnmadd213ss, "fmnadd213ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAD), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmadd231ss, "fmnadd231ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBD), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub132pd, "fmnsub132pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9E), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Packed Double-Precision Floating-Point Values -INST3(vfnmsub213pd, "fmnsub213pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAE), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub231pd, "fmnsub231pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBE), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub132ps, "fmnsub132ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9E), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Packed Single-Precision Floating-Point Values -INST3(vfnmsub213ps, "fmnsub213ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAE), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub231ps, "fmnsub231ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBE), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub132sd, "fmnsub132sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9F), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Scalar Double-Precision Floating-Point Values -INST3(vfnmsub213sd, "fmnsub213sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAF), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub231sd, "fmnsub231sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBF), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub132ss, "fmnsub132ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9F), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Scalar Single-Precision Floating-Point Values -INST3(vfnmsub213ss, "fmnsub213ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAF), INS_Flags_IsDstDstSrcAVXInstruction) // -INST3(vfnmsub231ss, "fmnsub231ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBF), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd132pd, "fnmadd132pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9C), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Packed Double-Precision Floating-Point Values +INST3(vfnmadd213pd, "fnmadd213pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAC), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd231pd, "fnmadd231pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBC), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd132ps, "fnmadd132ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9C), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Packed Single-Precision Floating-Point Values +INST3(vfnmadd213ps, "fnmadd213ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAC), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd231ps, "fnmadd231ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBC), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd132sd, "fnmadd132sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9D), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Scalar Double-Precision Floating-Point Values +INST3(vfnmadd213sd, "fnmadd213sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAD), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd231sd, "fnmadd231sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBD), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd132ss, "fnmadd132ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9D), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Add of Scalar Single-Precision Floating-Point Values +INST3(vfnmadd213ss, "fnmadd213ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAD), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmadd231ss, "fnmadd231ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBD), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub132pd, "fnmsub132pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9E), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Packed Double-Precision Floating-Point Values +INST3(vfnmsub213pd, "fnmsub213pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAE), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub231pd, "fnmsub231pd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBE), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub132ps, "fnmsub132ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9E), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Packed Single-Precision Floating-Point Values +INST3(vfnmsub213ps, "fnmsub213ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAE), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub231ps, "fnmsub231ps", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBE), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub132sd, "fnmsub132sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9F), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Scalar Double-Precision Floating-Point Values +INST3(vfnmsub213sd, "fnmsub213sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAF), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub231sd, "fnmsub231sd", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBF), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub132ss, "fnmsub132ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0x9F), INS_Flags_IsDstDstSrcAVXInstruction) // Fused Negative Multiply-Subtract of Scalar Single-Precision Floating-Point Values +INST3(vfnmsub213ss, "fnmsub213ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xAF), INS_Flags_IsDstDstSrcAVXInstruction) // +INST3(vfnmsub231ss, "fnmsub231ss", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xBF), INS_Flags_IsDstDstSrcAVXInstruction) // INST3(LAST_FMA_INSTRUCTION, "LAST_FMA_INSTRUCTION", IUM_WR, BAD_CODE, BAD_CODE, BAD_CODE, INS_FLAGS_None) // BMI1 From 9be9a0c9038916bbceccd061e5a94a75431c9fd7 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 11 Oct 2019 16:46:45 +0300 Subject: [PATCH 06/20] Move to Lower --- src/jit/importer.cpp | 50 ++++++------------------------------- src/jit/lowerxarch.cpp | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 6fba2798f636..198618b51fe8 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -4029,48 +4029,14 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // Vector128.CreateScalar(z) // ).ToScalar(); - // Depending on additional GT_NEG nodes it can be: - // - // x * y + z -> NI_FMA_MultiplyAddScalar - // x * -y + z -> NI_FMA_MultiplyAddNegatedScalar - // -x * y + z -> NI_FMA_MultiplyAddNegatedScalar - // -x * -y + z -> NI_FMA_MultiplyAddScalar - // x * y - z -> NI_FMA_MultiplySubtractScalar - // x * -y - z -> NI_FMA_MultiplySubtractNegatedScalar - // -x * y - z -> NI_FMA_MultiplySubtractNegatedScalar - // -x * -y - z -> NI_FMA_MultiplySubtractScalar - - GenTree* z = impPopStack().val; - GenTree* y = impPopStack().val; - GenTree* x = impPopStack().val; - - bool negMul = x->OperIs(GT_NEG) ^ y->OperIs(GT_NEG); - NamedIntrinsic fma; - if (x->OperIs(GT_NEG)) - { - x = x->gtGetOp1(); - } - if (y->OperIs(GT_NEG)) - { - y = y->gtGetOp1(); - } - if (z->OperIs(GT_NEG)) - { - z = z->gtGetOp1(); - fma = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; - } - else - { - fma = negMul ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; - } - - GenTree* op3 = - gtNewSimdHWIntrinsicNode(TYP_SIMD16, z, NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op2 = - gtNewSimdHWIntrinsicNode(TYP_SIMD16, y, NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* op1 = - gtNewSimdHWIntrinsicNode(TYP_SIMD16, x, NI_Vector128_CreateScalarUnsafe, callType, 16); - GenTree* res = gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, fma, callType, 16); + GenTree* op3 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, impPopStack().val, + NI_Vector128_CreateScalarUnsafe, callType, 16); + GenTree* op2 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, impPopStack().val, + NI_Vector128_CreateScalarUnsafe, callType, 16); + GenTree* op1 = gtNewSimdHWIntrinsicNode(TYP_SIMD16, impPopStack().val, + NI_Vector128_CreateScalarUnsafe, callType, 16); + GenTree* res = + gtNewSimdHWIntrinsicNode(TYP_SIMD16, op1, op2, op3, NI_FMA_MultiplyAddScalar, callType, 16); retNode = gtNewSimdHWIntrinsicNode(callType, res, NI_Vector128_ToScalar, callType, 16); } diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index e74245ab0eed..1b47907f0043 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -978,6 +978,62 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) LowerHWIntrinsicCC(node, NI_AVX_PTEST, GenCondition::UGT); break; + case NI_FMA_MultiplyAddScalar: { + // Math(F).FusedMultiplyAdd is expanded into NI_FMA_MultiplyAddScalar but + // depending on additional GT_NEG nodes it can be: + // + // x * y + z -> NI_FMA_MultiplyAddScalar + // x * -y + z -> NI_FMA_MultiplyAddNegatedScalar + // -x * y + z -> NI_FMA_MultiplyAddNegatedScalar + // -x * -y + z -> NI_FMA_MultiplyAddScalar + // x * y - z -> NI_FMA_MultiplySubtractScalar + // x * -y - z -> NI_FMA_MultiplySubtractNegatedScalar + // -x * y - z -> NI_FMA_MultiplySubtractNegatedScalar + // -x * -y - z -> NI_FMA_MultiplySubtractScalar + // + GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); + if (argList->Current()->OperIsUnary()) + { + GenTreeUnOp* createScalarOp1 = argList->Current()->AsUnOp(); + GenTree* argX = createScalarOp1->gtGetOp1(); + argList = argList->Rest(); + if (argList->Current()->OperIsUnary()) + { + GenTreeUnOp* createScalarOp2 = argList->Current()->AsUnOp(); + GenTree* argY = createScalarOp2->gtGetOp1(); + argList = argList->Rest(); + if (argList->Current()->OperIsUnary()) + { + GenTreeUnOp* createScalarOp3 = argList->Current()->AsUnOp(); + GenTree* argZ = createScalarOp3->gtGetOp1(); + bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); + if (argX->OperIs(GT_NEG)) + { + GenTree* arg = argX->gtGetOp1(); + // TODO: replace argX with arg + } + if (argY->OperIs(GT_NEG)) + { + GenTree* arg = argY->gtGetOp1(); + // TODO: replace argY with arg + } + if (argZ->OperIs(GT_NEG)) + { + GenTree* arg = argZ->gtGetOp1(); + // TODO: replace argZ with arg + node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; + } + else + { + node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; + } + } + } + } + + break; + } + default: break; } From 2033b04758c220acbc345552778e0122c40112ab Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 11 Oct 2019 17:00:07 +0300 Subject: [PATCH 07/20] clean up --- src/jit/lowerxarch.cpp | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index 1b47907f0043..fadbeb505276 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -992,35 +992,48 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) // -x * -y - z -> NI_FMA_MultiplySubtractScalar // GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); - if (argList->Current()->OperIsUnary()) + if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected { - GenTreeUnOp* createScalarOp1 = argList->Current()->AsUnOp(); - GenTree* argX = createScalarOp1->gtGetOp1(); + GenTreeUnOp* createScalarOpX = argList->Current()->AsUnOp(); + GenTree* argX = createScalarOpX->gtGetOp1(); argList = argList->Rest(); - if (argList->Current()->OperIsUnary()) + if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected { - GenTreeUnOp* createScalarOp2 = argList->Current()->AsUnOp(); - GenTree* argY = createScalarOp2->gtGetOp1(); + GenTreeUnOp* createScalarOpY = argList->Current()->AsUnOp(); + GenTree* argY = createScalarOpY->gtGetOp1(); argList = argList->Rest(); - if (argList->Current()->OperIsUnary()) + if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected { - GenTreeUnOp* createScalarOp3 = argList->Current()->AsUnOp(); - GenTree* argZ = createScalarOp3->gtGetOp1(); + GenTreeUnOp* createScalarOpZ = argList->Current()->AsUnOp(); + GenTree* argZ = createScalarOpZ->gtGetOp1(); bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); if (argX->OperIs(GT_NEG)) { GenTree* arg = argX->gtGetOp1(); - // TODO: replace argX with arg + + // Drop GT_NEG + // TODO: this doesn't work: + arg->gtNext = createScalarOpX; + createScalarOpX->gtPrev = arg; } if (argY->OperIs(GT_NEG)) { GenTree* arg = argY->gtGetOp1(); - // TODO: replace argY with arg + + // Drop GT_NEG + // TODO: this doesn't work: + arg->gtNext = createScalarOpY; + createScalarOpY->gtPrev = arg; } if (argZ->OperIs(GT_NEG)) { GenTree* arg = argZ->gtGetOp1(); - // TODO: replace argZ with arg + + // Drop GT_NEG + // TODO: this doesn't work: + arg->gtNext = createScalarOpY; + createScalarOpY->gtPrev = arg; + node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else From 0d2559317c38bfcf7af9844071686ca2bcdef22f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 11 Oct 2019 17:33:32 +0300 Subject: [PATCH 08/20] Fix assert --- src/jit/lowerxarch.cpp | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index fadbeb505276..7d9b7abb3631 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -1009,31 +1009,18 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); if (argX->OperIs(GT_NEG)) { - GenTree* arg = argX->gtGetOp1(); - - // Drop GT_NEG - // TODO: this doesn't work: - arg->gtNext = createScalarOpX; - createScalarOpX->gtPrev = arg; + BlockRange().Remove(argX); + createScalarOpX->gtOp1 = argX->gtGetOp1(); } if (argY->OperIs(GT_NEG)) { - GenTree* arg = argY->gtGetOp1(); - - // Drop GT_NEG - // TODO: this doesn't work: - arg->gtNext = createScalarOpY; - createScalarOpY->gtPrev = arg; + BlockRange().Remove(argY); + createScalarOpY->gtOp1 = argY->gtGetOp1(); } if (argZ->OperIs(GT_NEG)) { - GenTree* arg = argZ->gtGetOp1(); - - // Drop GT_NEG - // TODO: this doesn't work: - arg->gtNext = createScalarOpY; - createScalarOpY->gtPrev = arg; - + BlockRange().Remove(argZ); + createScalarOpZ->gtOp1 = argZ->gtGetOp1(); node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else From b4abe2d4fc9ec3ad43d9c264dc11a22a6111e026 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 11 Oct 2019 18:01:52 +0300 Subject: [PATCH 09/20] code formatting --- src/jit/lowerxarch.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index 7d9b7abb3631..f08c43c75214 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -978,7 +978,8 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) LowerHWIntrinsicCC(node, NI_AVX_PTEST, GenCondition::UGT); break; - case NI_FMA_MultiplyAddScalar: { + case NI_FMA_MultiplyAddScalar: + { // Math(F).FusedMultiplyAdd is expanded into NI_FMA_MultiplyAddScalar but // depending on additional GT_NEG nodes it can be: // @@ -995,18 +996,18 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected { GenTreeUnOp* createScalarOpX = argList->Current()->AsUnOp(); - GenTree* argX = createScalarOpX->gtGetOp1(); + GenTree* argX = createScalarOpX->gtGetOp1(); argList = argList->Rest(); if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected { GenTreeUnOp* createScalarOpY = argList->Current()->AsUnOp(); - GenTree* argY = createScalarOpY->gtGetOp1(); + GenTree* argY = createScalarOpY->gtGetOp1(); argList = argList->Rest(); if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected { GenTreeUnOp* createScalarOpZ = argList->Current()->AsUnOp(); - GenTree* argZ = createScalarOpZ->gtGetOp1(); - bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); + GenTree* argZ = createScalarOpZ->gtGetOp1(); + bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); if (argX->OperIs(GT_NEG)) { BlockRange().Remove(argX); @@ -1021,7 +1022,8 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) { BlockRange().Remove(argZ); createScalarOpZ->gtOp1 = argZ->gtGetOp1(); - node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; + node->gtHWIntrinsicId = + negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else { @@ -1030,7 +1032,6 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) } } } - break; } From 23de54317c96b7f58fe99ec1b5976693c0dc8852 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 11 Oct 2019 18:16:05 +0300 Subject: [PATCH 10/20] code formatting --- src/jit/lowerxarch.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index f08c43c75214..57bb1518eff3 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -1006,8 +1006,8 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected { GenTreeUnOp* createScalarOpZ = argList->Current()->AsUnOp(); - GenTree* argZ = createScalarOpZ->gtGetOp1(); - bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); + GenTree* argZ = createScalarOpZ->gtGetOp1(); + bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); if (argX->OperIs(GT_NEG)) { BlockRange().Remove(argX); From 708dc3916fbc9a27f03f8ddeb67e6a185180fcb3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 25 Oct 2019 13:19:22 +0300 Subject: [PATCH 11/20] Address feedback --- src/jit/lower.h | 1 + src/jit/lowerxarch.cpp | 138 +++++++++++++++++++++++++---------------- 2 files changed, 85 insertions(+), 54 deletions(-) diff --git a/src/jit/lower.h b/src/jit/lower.h index 4fe552918728..cad728a76269 100644 --- a/src/jit/lower.h +++ b/src/jit/lower.h @@ -310,6 +310,7 @@ class Lowering : public Phase #ifdef FEATURE_HW_INTRINSICS void LowerHWIntrinsic(GenTreeHWIntrinsic* node); void LowerHWIntrinsicCC(GenTreeHWIntrinsic* node, NamedIntrinsic newIntrinsicId, GenCondition condition); + void LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node); #endif // FEATURE_HW_INTRINSICS // Utility functions diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index 1ac83759cb9b..cde0a03afd1d 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -805,6 +805,89 @@ void Lowering::LowerHWIntrinsicCC(GenTreeHWIntrinsic* node, NamedIntrinsic newIn } } +//---------------------------------------------------------------------------------------------- +// LowerFusedMultiplyAdd: Changes NI_FMA_MultiplyAddScalar produced by Math(F).FusedMultiplyAdd +// to a better FMA intrinsics if there are GT_NEG around in order to eliminate them. +// +// Arguments: +// node - The hardware intrinsic node +// +// Notes: +// Math(F).FusedMultiplyAdd is expanded into NI_FMA_MultiplyAddScalar and +// depending on additional GT_NEG nodes around it can be: +// +// x * y + z -> NI_FMA_MultiplyAddScalar +// x * -y + z -> NI_FMA_MultiplyAddNegatedScalar +// -x * y + z -> NI_FMA_MultiplyAddNegatedScalar +// -x * -y + z -> NI_FMA_MultiplyAddScalar +// x * y - z -> NI_FMA_MultiplySubtractScalar +// x * -y - z -> NI_FMA_MultiplySubtractNegatedScalar +// -x * y - z -> NI_FMA_MultiplySubtractNegatedScalar +// -x * -y - z -> NI_FMA_MultiplySubtractScalar +// +void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) +{ + GenTreeHWIntrinsic* createScalarOpX = nullptr; + GenTreeHWIntrinsic* createScalarOpY = nullptr; + GenTreeHWIntrinsic* createScalarOpZ = nullptr; + GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); + + for (size_t i = 0; i < 3; i++) + { + assert(argList->Current() != nullptr); + if (argList->Current()->OperIsHWIntrinsic()) + { + GenTreeHWIntrinsic* hwArg = argList->Current()->AsHWIntrinsic(); + if (hwArg->gtHWIntrinsicId == NI_Vector128_CreateScalarUnsafe) + { + if (i == 0) + { + createScalarOpX = hwArg; + } + else if (i == 1) + { + createScalarOpY = hwArg; + } + else + { + createScalarOpZ = hwArg; + } + argList = argList->Rest(); + continue; + } + } + return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe + // but it's also possible to use NI_FMA_MultiplyAddScalar directly + } + + GenTree* argX = createScalarOpX->gtGetOp1(); + GenTree* argY = createScalarOpY->gtGetOp1(); + GenTree* argZ = createScalarOpZ->gtGetOp1(); + + const bool negMul = argX->OperIs(GT_NEG) != argY->OperIs(GT_NEG); + if (argX->OperIs(GT_NEG)) + { + BlockRange().Remove(argX); + createScalarOpX->gtOp1 = argX->gtGetOp1(); + } + if (argY->OperIs(GT_NEG)) + { + BlockRange().Remove(argY); + createScalarOpY->gtOp1 = argY->gtGetOp1(); + } + if (argZ->OperIs(GT_NEG)) + { + BlockRange().Remove(argZ); + createScalarOpZ->gtOp1 = argZ->gtGetOp1(); + node->gtHWIntrinsicId = + negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; + } + else + { + node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; + } +} + //---------------------------------------------------------------------------------------------- // Lowering::LowerHWIntrinsic: Perform containment analysis for a hardware intrinsic node. // @@ -912,61 +995,8 @@ void Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) break; case NI_FMA_MultiplyAddScalar: - { - // Math(F).FusedMultiplyAdd is expanded into NI_FMA_MultiplyAddScalar but - // depending on additional GT_NEG nodes it can be: - // - // x * y + z -> NI_FMA_MultiplyAddScalar - // x * -y + z -> NI_FMA_MultiplyAddNegatedScalar - // -x * y + z -> NI_FMA_MultiplyAddNegatedScalar - // -x * -y + z -> NI_FMA_MultiplyAddScalar - // x * y - z -> NI_FMA_MultiplySubtractScalar - // x * -y - z -> NI_FMA_MultiplySubtractNegatedScalar - // -x * y - z -> NI_FMA_MultiplySubtractNegatedScalar - // -x * -y - z -> NI_FMA_MultiplySubtractScalar - // - GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); - if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected - { - GenTreeUnOp* createScalarOpX = argList->Current()->AsUnOp(); - GenTree* argX = createScalarOpX->gtGetOp1(); - argList = argList->Rest(); - if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected - { - GenTreeUnOp* createScalarOpY = argList->Current()->AsUnOp(); - GenTree* argY = createScalarOpY->gtGetOp1(); - argList = argList->Rest(); - if (argList->Current()->OperIsHWIntrinsic()) // CreateScalarUnsafe is expected - { - GenTreeUnOp* createScalarOpZ = argList->Current()->AsUnOp(); - GenTree* argZ = createScalarOpZ->gtGetOp1(); - bool negMul = argX->OperIs(GT_NEG) ^ argY->OperIs(GT_NEG); - if (argX->OperIs(GT_NEG)) - { - BlockRange().Remove(argX); - createScalarOpX->gtOp1 = argX->gtGetOp1(); - } - if (argY->OperIs(GT_NEG)) - { - BlockRange().Remove(argY); - createScalarOpY->gtOp1 = argY->gtGetOp1(); - } - if (argZ->OperIs(GT_NEG)) - { - BlockRange().Remove(argZ); - createScalarOpZ->gtOp1 = argZ->gtGetOp1(); - node->gtHWIntrinsicId = - negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; - } - else - { - node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplyAddNegatedScalar : NI_FMA_MultiplyAddScalar; - } - } - } - } + LowerFusedMultiplyAdd(node); break; - } default: break; From 5e3138d501d0f5627928aeb561c7cf39d35bb34a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 25 Oct 2019 13:38:24 +0300 Subject: [PATCH 12/20] Clean up --- src/jit/lowerxarch.cpp | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index cde0a03afd1d..f4b4757966c0 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -827,12 +827,10 @@ void Lowering::LowerHWIntrinsicCC(GenTreeHWIntrinsic* node, NamedIntrinsic newIn // void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) { - GenTreeHWIntrinsic* createScalarOpX = nullptr; - GenTreeHWIntrinsic* createScalarOpY = nullptr; - GenTreeHWIntrinsic* createScalarOpZ = nullptr; - GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); + GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); + GenTreeHWIntrinsic* createScalarOps[3]; - for (size_t i = 0; i < 3; i++) + for (auto& createScalarOp : createScalarOps) { assert(argList->Current() != nullptr); if (argList->Current()->OperIsHWIntrinsic()) @@ -840,47 +838,36 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) GenTreeHWIntrinsic* hwArg = argList->Current()->AsHWIntrinsic(); if (hwArg->gtHWIntrinsicId == NI_Vector128_CreateScalarUnsafe) { - if (i == 0) - { - createScalarOpX = hwArg; - } - else if (i == 1) - { - createScalarOpY = hwArg; - } - else - { - createScalarOpZ = hwArg; - } + createScalarOp = hwArg; argList = argList->Rest(); continue; } } + return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe // but it's also possible to use NI_FMA_MultiplyAddScalar directly } - GenTree* argX = createScalarOpX->gtGetOp1(); - GenTree* argY = createScalarOpY->gtGetOp1(); - GenTree* argZ = createScalarOpZ->gtGetOp1(); + GenTree* argX = createScalarOps[0]->gtGetOp1(); + GenTree* argY = createScalarOps[1]->gtGetOp1(); + GenTree* argZ = createScalarOps[2]->gtGetOp1(); const bool negMul = argX->OperIs(GT_NEG) != argY->OperIs(GT_NEG); if (argX->OperIs(GT_NEG)) { BlockRange().Remove(argX); - createScalarOpX->gtOp1 = argX->gtGetOp1(); + createScalarOps[0]->gtOp1 = argX->gtGetOp1(); } if (argY->OperIs(GT_NEG)) { BlockRange().Remove(argY); - createScalarOpY->gtOp1 = argY->gtGetOp1(); + createScalarOps[1]->gtOp1 = argY->gtGetOp1(); } if (argZ->OperIs(GT_NEG)) { BlockRange().Remove(argZ); - createScalarOpZ->gtOp1 = argZ->gtGetOp1(); - node->gtHWIntrinsicId = - negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; + createScalarOps[2]->gtOp1 = argZ->gtGetOp1(); + node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else { From 77afb8cada7b2239774375d797fc4850092910de Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 25 Oct 2019 13:55:57 +0300 Subject: [PATCH 13/20] Add an assert to make sure we only have 3 args --- src/jit/lowerxarch.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index f4b4757966c0..3a7c31de6224 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -839,7 +839,7 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) if (hwArg->gtHWIntrinsicId == NI_Vector128_CreateScalarUnsafe) { createScalarOp = hwArg; - argList = argList->Rest(); + argList = argList->Rest(); continue; } } @@ -847,6 +847,7 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe // but it's also possible to use NI_FMA_MultiplyAddScalar directly } + assert(argList == nullptr); GenTree* argX = createScalarOps[0]->gtGetOp1(); GenTree* argY = createScalarOps[1]->gtGetOp1(); From 8e49971018dc46c482ee028564a979827a7daaac Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 25 Oct 2019 18:07:36 +0300 Subject: [PATCH 14/20] Address feedback and add tests --- src/jit/lowerxarch.cpp | 20 +- .../JIT/Intrinsics/MathFusedMultiplyAdd.cs | 186 ++++++++++++++++++ .../Intrinsics/MathFusedMultiplyAdd_r.csproj | 13 ++ .../Intrinsics/MathFusedMultiplyAdd_ro.csproj | 13 ++ 4 files changed, 222 insertions(+), 10 deletions(-) create mode 100644 tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs create mode 100644 tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj create mode 100644 tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index 3a7c31de6224..9d4c4e341564 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -828,9 +828,9 @@ void Lowering::LowerHWIntrinsicCC(GenTreeHWIntrinsic* node, NamedIntrinsic newIn void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) { GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); - GenTreeHWIntrinsic* createScalarOps[3]; + GenTreeHWIntrinsic* createScalarOps[4]; // 3 arguments with 1-based index - for (auto& createScalarOp : createScalarOps) + for (int i = 1; i < 4; i++) { assert(argList->Current() != nullptr); if (argList->Current()->OperIsHWIntrinsic()) @@ -838,8 +838,8 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) GenTreeHWIntrinsic* hwArg = argList->Current()->AsHWIntrinsic(); if (hwArg->gtHWIntrinsicId == NI_Vector128_CreateScalarUnsafe) { - createScalarOp = hwArg; - argList = argList->Rest(); + createScalarOps[i] = hwArg; + argList = argList->Rest(); continue; } } @@ -849,25 +849,25 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) } assert(argList == nullptr); - GenTree* argX = createScalarOps[0]->gtGetOp1(); - GenTree* argY = createScalarOps[1]->gtGetOp1(); - GenTree* argZ = createScalarOps[2]->gtGetOp1(); + GenTree* argX = createScalarOps[1]->gtGetOp1(); + GenTree* argY = createScalarOps[2]->gtGetOp1(); + GenTree* argZ = createScalarOps[3]->gtGetOp1(); const bool negMul = argX->OperIs(GT_NEG) != argY->OperIs(GT_NEG); if (argX->OperIs(GT_NEG)) { BlockRange().Remove(argX); - createScalarOps[0]->gtOp1 = argX->gtGetOp1(); + createScalarOps[1]->gtOp1 = argX->gtGetOp1(); } if (argY->OperIs(GT_NEG)) { BlockRange().Remove(argY); - createScalarOps[1]->gtOp1 = argY->gtGetOp1(); + createScalarOps[2]->gtOp1 = argY->gtGetOp1(); } if (argZ->OperIs(GT_NEG)) { BlockRange().Remove(argZ); - createScalarOps[2]->gtOp1 = argZ->gtGetOp1(); + createScalarOps[3]->gtOp1 = argZ->gtGetOp1(); node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else diff --git a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs new file mode 100644 index 000000000000..dca0678e58a1 --- /dev/null +++ b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs @@ -0,0 +1,186 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. +// + +using System; +using System.Runtime.CompilerServices; + +namespace MathFusedMultiplyAddTest +{ + class Program + { + private static int _returnCode = 100; + + static int Main() + { + TestFloats(); + TestDoubles(); + return _returnCode; + } + +#region MathF.FusedMultiplyAdd + static void TestFloats() + { + float[] testValues = + { + MathF.PI, MathF.E, 0.0f, -0.0f, float.MinValue, float.MaxValue, 42, -42, 1000, -1000, + int.MaxValue, int.MinValue, float.NaN, float.PositiveInfinity, float.NegativeInfinity + }; + + foreach (float a in testValues) + { + foreach (float b in testValues) + { + foreach (float c in testValues) + { + Check1(a, b, c); + Check2(a, b, c); + Check3(a, b, c); + Check4(a, b, c); + Check5(a, b, c); + Check6(a, b, c); + Check7(a, b, c); + Check8(a, b, c); + } + } + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check1(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd( a, b, c), + MathF.FusedMultiplyAdd( a, b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check2(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd(-a, b, c), + MathF.FusedMultiplyAdd(-a, b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check3(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd(-a, -b, c), + MathF.FusedMultiplyAdd(-a, -b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check4(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd(-a, -b, -c), + MathF.FusedMultiplyAdd(-a, -b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check5(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd( a, -b, c), + MathF.FusedMultiplyAdd( a, -b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check6(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd( a, -b, -c), + MathF.FusedMultiplyAdd( a, -b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check7(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd(-a, b, -c), + MathF.FusedMultiplyAdd(-a, b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check8(float a, float b, float c) => + CompareFloats(ReferenceMultiplyAdd( a, b, -c), + MathF.FusedMultiplyAdd( a, b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static float ReferenceMultiplyAdd(float a, float b, float c) => a * b + c; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void CompareFloats(float a, float b) + { + if (Math.Abs(a - b) > 0.001f) + { + Console.WriteLine($"{a} != {b}"); + _returnCode--; + } + } +#endregion + +#region Math.FusedMultiplyAdd + static void TestDoubles() + { + double[] testValues = + { + Math.PI, Math.E, 0.0, -0.0, double.MinValue, double.MaxValue, 42, -42, 100000, -100000, + long.MaxValue, long.MinValue, double.NaN, double.PositiveInfinity, double.NegativeInfinity + }; + + foreach (double a in testValues) + { + foreach (double b in testValues) + { + foreach (double c in testValues) + { + Check1(a, b, c); + Check2(a, b, c); + Check3(a, b, c); + Check4(a, b, c); + Check5(a, b, c); + Check6(a, b, c); + Check7(a, b, c); + Check8(a, b, c); + } + } + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check1(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd( a, b, c), + Math.FusedMultiplyAdd( a, b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check2(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd(-a, b, c), + Math.FusedMultiplyAdd(-a, b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check3(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd(-a, -b, c), + Math.FusedMultiplyAdd(-a, -b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check4(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd(-a, -b, -c), + Math.FusedMultiplyAdd(-a, -b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check5(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd( a, -b, c), + Math.FusedMultiplyAdd( a, -b, c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check6(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd( a, -b, -c), + Math.FusedMultiplyAdd( a, -b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check7(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd(-a, b, -c), + Math.FusedMultiplyAdd(-a, b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Check8(double a, double b, double c) => + CompareDoubles(ReferenceMultiplyAdd( a, b, -c), + Math.FusedMultiplyAdd( a, b, -c)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static double ReferenceMultiplyAdd(double a, double b, double c) => a * b + c; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void CompareDoubles(double a, double b) + { + if (Math.Abs(a - b) > 0.00001) + { + Console.WriteLine($"{a} != {b}"); + _returnCode--; + } + } +#endregion + } +} diff --git a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj new file mode 100644 index 000000000000..7928cb69a0b1 --- /dev/null +++ b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj @@ -0,0 +1,13 @@ + + + Exe + + + true + None + + + + + + diff --git a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj new file mode 100644 index 000000000000..1795200a5c9d --- /dev/null +++ b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj @@ -0,0 +1,13 @@ + + + Exe + + + true + None + True + + + + + From d938e5a030c88635291a29978c669bbd67a9760c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Nov 2019 15:46:06 +0300 Subject: [PATCH 15/20] Address feedback --- src/jit/lowerxarch.cpp | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index 885c1f4136e5..a12ee7b261e4 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -825,46 +825,45 @@ void Lowering::LowerHWIntrinsicCC(GenTreeHWIntrinsic* node, NamedIntrinsic newIn void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) { GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); - GenTreeHWIntrinsic* createScalarOps[4]; // 3 arguments with 1-based index + GenTreeHWIntrinsic* createScalarOps[3]; - for (int i = 1; i < 4; i++) + for (auto& createScalarOp : createScalarOps) { assert(argList->Current() != nullptr); if (argList->Current()->OperIsHWIntrinsic()) { GenTreeHWIntrinsic* hwArg = argList->Current()->AsHWIntrinsic(); - if (hwArg->gtHWIntrinsicId == NI_Vector128_CreateScalarUnsafe) + if (hwArg->gtHWIntrinsicId != NI_Vector128_CreateScalarUnsafe) { - createScalarOps[i] = hwArg; - argList = argList->Rest(); - continue; + return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe + // but it's also possible to use NI_FMA_MultiplyAddScalar directly with any } + createScalarOp = hwArg; + argList = argList->Rest(); } - return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe - // but it's also possible to use NI_FMA_MultiplyAddScalar directly } assert(argList == nullptr); - GenTree* argX = createScalarOps[1]->gtGetOp1(); - GenTree* argY = createScalarOps[2]->gtGetOp1(); - GenTree* argZ = createScalarOps[3]->gtGetOp1(); + GenTree* argX = createScalarOps[0]->gtGetOp1(); + GenTree* argY = createScalarOps[1]->gtGetOp1(); + GenTree* argZ = createScalarOps[2]->gtGetOp1(); const bool negMul = argX->OperIs(GT_NEG) != argY->OperIs(GT_NEG); if (argX->OperIs(GT_NEG)) { + createScalarOps[0]->gtOp1 = argX->gtGetOp1(); BlockRange().Remove(argX); - createScalarOps[1]->gtOp1 = argX->gtGetOp1(); } if (argY->OperIs(GT_NEG)) { + createScalarOps[1]->gtOp1 = argY->gtGetOp1(); BlockRange().Remove(argY); - createScalarOps[2]->gtOp1 = argY->gtGetOp1(); } if (argZ->OperIs(GT_NEG)) { + createScalarOps[2]->gtOp1 = argZ->gtGetOp1(); BlockRange().Remove(argZ); - createScalarOps[3]->gtOp1 = argZ->gtGetOp1(); node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else From 2de2885077a5a5d592828f312392c48ab4b08c4e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Nov 2019 16:25:24 +0300 Subject: [PATCH 16/20] update tests --- .../JIT/Intrinsics/MathFusedMultiplyAdd.cs | 140 +++++++++++++++++- 1 file changed, 137 insertions(+), 3 deletions(-) diff --git a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs index dca0678e58a1..3f117048f4c5 100644 --- a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs +++ b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd.cs @@ -5,6 +5,8 @@ using System; using System.Runtime.CompilerServices; +using System.Runtime.Intrinsics; +using System.Runtime.Intrinsics.X86; namespace MathFusedMultiplyAddTest { @@ -42,6 +44,16 @@ static void TestFloats() Check6(a, b, c); Check7(a, b, c); Check8(a, b, c); + + if (Fma.IsSupported) + { + Vector128 vecA = Vector128.Create(42f); + TestExplicitFmaUsage1(ref vecA, 9f); + TestExplicitFmaUsage2(ref vecA, 9f); + TestExplicitFmaUsage3(ref vecA, 9f); + TestExplicitFmaUsage4(ref vecA, 9f); + TestExplicitFmaUsage5(ref vecA, 9f); + } } } } @@ -99,9 +111,65 @@ static void CompareFloats(float a, float b) _returnCode--; } } -#endregion -#region Math.FusedMultiplyAdd + // FMA intrinsics can be used explicitly, make sure nothing asserts + // with various types of arguments (fields, local variables, constants and refs) + + static Vector128 _c32 = Vector128.CreateScalarUnsafe(MathF.PI); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage1(ref Vector128 a, float b) + { + CompareFloats(ReferenceMultiplyAdd(a.ToScalar(), b, _c32.ToScalar()), + Fma.MultiplyAdd(a, Vector128.CreateScalarUnsafe(b), _c32).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage2(ref Vector128 a, float b) + { + CompareFloats(ReferenceMultiplyAdd(a.ToScalar(), a.ToScalar(), a.ToScalar()), + Fma.MultiplyAdd(a, a, a).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage3(ref Vector128 a, float b) + { + CompareFloats(ReferenceMultiplyAdd(_c32.ToScalar(), _c32.ToScalar(), _c32.ToScalar()), + Fma.MultiplyAdd(_c32, _c32, _c32).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage4(ref Vector128 a, float b) + { + CompareFloats(ReferenceMultiplyAdd(b, b, 333f), + Fma.MultiplyAdd( + Vector128.CreateScalarUnsafe(b), + Vector128.CreateScalarUnsafe(b), + Vector128.CreateScalarUnsafe(333f)).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage5(ref Vector128 a, float b) + { + CompareDoubles(ReferenceMultiplyAdd(-b, -b, -333f), + Fma.MultiplyAdd( + Vector128.CreateScalarUnsafe(-b), + Vector128.CreateScalarUnsafe(-b), + Vector128.CreateScalarUnsafe(-333f)).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage6(ref Vector128 a, float b) + { + CompareFloats(ReferenceMultiplyAdd(b, b, b), + Fma.MultiplyAdd( + Vector128.CreateScalarUnsafe(b), + Vector128.CreateScalar(b), + Vector128.Create(b)).ToScalar()); + } + #endregion + + #region Math.FusedMultiplyAdd static void TestDoubles() { double[] testValues = @@ -124,6 +192,16 @@ static void TestDoubles() Check6(a, b, c); Check7(a, b, c); Check8(a, b, c); + + if (Fma.IsSupported) + { + Vector128 vecA = Vector128.Create(42.0); + TestExplicitFmaUsage1(ref vecA, 9f); + TestExplicitFmaUsage2(ref vecA, 9f); + TestExplicitFmaUsage3(ref vecA, 9f); + TestExplicitFmaUsage4(ref vecA, 9f); + TestExplicitFmaUsage5(ref vecA, 9f); + } } } } @@ -181,6 +259,62 @@ static void CompareDoubles(double a, double b) _returnCode--; } } + + // FMA intrinsics can be used explicitly, make sure nothing asserts + // with various types of arguments (fields, local variables, constants and refs) + + static Vector128 _c64 = Vector128.CreateScalarUnsafe(Math.PI); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage1(ref Vector128 a, double b) + { + CompareDoubles(ReferenceMultiplyAdd(a.ToScalar(), b, _c64.ToScalar()), + Fma.MultiplyAdd(a, Vector128.CreateScalarUnsafe(b), _c64).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage2(ref Vector128 a, double b) + { + CompareDoubles(ReferenceMultiplyAdd(a.ToScalar(), a.ToScalar(), a.ToScalar()), + Fma.MultiplyAdd(a, a, a).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage3(ref Vector128 a, double b) + { + CompareDoubles(ReferenceMultiplyAdd(_c64.ToScalar(), _c64.ToScalar(), _c64.ToScalar()), + Fma.MultiplyAdd(_c64, _c64, _c64).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage4(ref Vector128 a, double b) + { + CompareDoubles(ReferenceMultiplyAdd(b, b, b), + Fma.MultiplyAdd( + Vector128.CreateScalarUnsafe(b), + Vector128.CreateScalarUnsafe(b), + Vector128.CreateScalarUnsafe(b)).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage5(ref Vector128 a, double b) + { + CompareDoubles(ReferenceMultiplyAdd(-b, -b, -333.0), + Fma.MultiplyAdd( + Vector128.CreateScalarUnsafe(-b), + Vector128.CreateScalarUnsafe(-b), + Vector128.CreateScalarUnsafe(-333.0)).ToScalar()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void TestExplicitFmaUsage6(ref Vector128 a, double b) + { + CompareDoubles(ReferenceMultiplyAdd(b, b, b), + Fma.MultiplyAdd( + Vector128.CreateScalarUnsafe(b), + Vector128.CreateScalar(b), + Vector128.Create(b)).ToScalar()); + } #endregion } -} +} \ No newline at end of file From a8c0a36a0feb1f52451a7f850c3b28ae7712f1d9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Nov 2019 17:30:50 +0300 Subject: [PATCH 17/20] Fix CI failure --- tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj | 2 +- tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj index 7928cb69a0b1..879a577c27c3 100644 --- a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj +++ b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_r.csproj @@ -8,6 +8,6 @@ - + diff --git a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj index 1795200a5c9d..c7a857e226f7 100644 --- a/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj +++ b/tests/src/JIT/Intrinsics/MathFusedMultiplyAdd_ro.csproj @@ -8,6 +8,6 @@ True - + From f558b44b95d3be6269a5306db678e5b73380b7cf Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Nov 2019 20:00:02 +0300 Subject: [PATCH 18/20] Address feedback --- src/jit/lowerxarch.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index a12ee7b261e4..a70a460b1378 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -827,21 +827,21 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); GenTreeHWIntrinsic* createScalarOps[3]; - for (auto& createScalarOp : createScalarOps) + for (GenTreeHWIntrinsic*& createScalarOp : createScalarOps) { - assert(argList->Current() != nullptr); - if (argList->Current()->OperIsHWIntrinsic()) + GenTree*& current = argList->Current(); + assert(current != nullptr); + if (current->OperIsHWIntrinsic()) { - GenTreeHWIntrinsic* hwArg = argList->Current()->AsHWIntrinsic(); + GenTreeHWIntrinsic* hwArg = current->AsHWIntrinsic(); if (hwArg->gtHWIntrinsicId != NI_Vector128_CreateScalarUnsafe) { return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe - // but it's also possible to use NI_FMA_MultiplyAddScalar directly with any + // but it's also possible to use NI_FMA_MultiplyAddScalar directly with any operands } createScalarOp = hwArg; argList = argList->Rest(); } - } assert(argList == nullptr); From bed3fda1aa2186e988947ec35c8176c8e68cae8c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 4 Nov 2019 20:31:37 +0300 Subject: [PATCH 19/20] formatting --- src/jit/lowerxarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index a70a460b1378..d3ba102065a8 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -864,7 +864,7 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) { createScalarOps[2]->gtOp1 = argZ->gtGetOp1(); BlockRange().Remove(argZ); - node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; + node->gtHWIntrinsicId = negMul ? NI_FMA_MultiplySubtractNegatedScalar : NI_FMA_MultiplySubtractScalar; } else { From 9267af252ab9dab61c759d3a2cf60936e3f740cf Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 5 Nov 2019 01:46:25 +0300 Subject: [PATCH 20/20] Fix failures --- src/jit/lowerxarch.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index d3ba102065a8..743d7ac523bc 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -824,6 +824,7 @@ void Lowering::LowerHWIntrinsicCC(GenTreeHWIntrinsic* node, NamedIntrinsic newIn // void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) { + assert(node->gtHWIntrinsicId == NI_FMA_MultiplyAddScalar); GenTreeArgList* argList = node->gtGetOp1()->AsArgList(); GenTreeHWIntrinsic* createScalarOps[3]; @@ -831,17 +832,18 @@ void Lowering::LowerFusedMultiplyAdd(GenTreeHWIntrinsic* node) { GenTree*& current = argList->Current(); assert(current != nullptr); - if (current->OperIsHWIntrinsic()) + if (!current->OperIsHWIntrinsic()) { - GenTreeHWIntrinsic* hwArg = current->AsHWIntrinsic(); - if (hwArg->gtHWIntrinsicId != NI_Vector128_CreateScalarUnsafe) - { - return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe - // but it's also possible to use NI_FMA_MultiplyAddScalar directly with any operands - } - createScalarOp = hwArg; - argList = argList->Rest(); + return; // Math(F).FusedMultiplyAdd is expected to emit three NI_Vector128_CreateScalarUnsafe + // but it's also possible to use NI_FMA_MultiplyAddScalar directly with any operands + } + GenTreeHWIntrinsic* hwArg = current->AsHWIntrinsic(); + if (hwArg->gtHWIntrinsicId != NI_Vector128_CreateScalarUnsafe) + { + return; } + createScalarOp = hwArg; + argList = argList->Rest(); } assert(argList == nullptr);