[Qemu-devel] [PATCH v2] target/ppc: rewrite f[n]m[add, sub] using float64_muladd

Nikunj A Dadhania posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1488463829-26839-1-git-send-email-nikunj@linux.vnet.ibm.com
Test checkpatch passed
Test docker passed
Test s390x passed
target/ppc/fpu_helper.c | 213 +++++++++++-------------------------------------
1 file changed, 46 insertions(+), 167 deletions(-)
[Qemu-devel] [PATCH v2] target/ppc: rewrite f[n]m[add, sub] using float64_muladd
Posted by Nikunj A Dadhania 7 years ago
Use the softfloat api for fused multiply-add.
Introduce routine to set the FPSCR flags VXNAN, VXIMZ nad VMISI.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

---

v1:
* Removed float64_madd_set_vxisi()
* Introduced float64_maddsub_update_excp() will updated required flags
* Changed layout of error handling

v0:
* Use MADD/MSUB_FLAGS as used by VSX instructions
* Introduce helper float64_madd_set_vxisi()
---
 target/ppc/fpu_helper.c | 213 +++++++++++-------------------------------------
 1 file changed, 46 insertions(+), 167 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 58aee64..0535ad0 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -743,178 +743,62 @@ uint64_t helper_frim(CPUPPCState *env, uint64_t arg)
     return do_fri(env, arg, float_round_down);
 }
 
-/* fmadd - fmadd. */
-uint64_t helper_fmadd(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
-                      uint64_t arg3)
+static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
+                                        float64 arg2, float64 arg3,
+                                        unsigned int madd_flags)
 {
-    CPU_DoubleU farg1, farg2, farg3;
-
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-    farg3.ll = arg3;
-
-    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
-        /* Multiplication of zero by infinity */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
-            /* sNaN operation */
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
-        }
-        /* This is the way the PowerPC specification defines it */
-        float128 ft0_128, ft1_128;
-
-        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
-        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
-        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
-        if (unlikely(float128_is_infinity(ft0_128) &&
-                     float64_is_infinity(farg3.d) &&
-                     float128_is_neg(ft0_128) != float64_is_neg(farg3.d))) {
-            /* Magnitude subtraction of infinities */
-            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
-        } else {
-            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
-            ft0_128 = float128_add(ft0_128, ft1_128, &env->fp_status);
-            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
-        }
-    }
-
-    return farg1.ll;
-}
-
-/* fmsub - fmsub. */
-uint64_t helper_fmsub(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
-                      uint64_t arg3)
-{
-    CPU_DoubleU farg1, farg2, farg3;
-
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-    farg3.ll = arg3;
-
-    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                 (float64_is_zero(farg1.d) &&
-                  float64_is_infinity(farg2.d)))) {
+    if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
+                 (float64_is_zero(arg1) && float64_is_infinity(arg2)))) {
         /* Multiplication of zero by infinity */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
-            /* sNaN operation */
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+        arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
+    } else if (unlikely(float64_is_signaling_nan(arg1, &env->fp_status) ||
+                        float64_is_signaling_nan(arg2, &env->fp_status) ||
+                        float64_is_signaling_nan(arg3, &env->fp_status))) {
+        /* sNaN operation */
+        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+    } else if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
+               float64_is_infinity(arg3)) {
+        uint8_t aSign, bSign, cSign;
+
+        aSign = float64_is_neg(arg1);
+        bSign = float64_is_neg(arg2);
+        cSign = float64_is_neg(arg3);
+        if (madd_flags & float_muladd_negate_c) {
+            cSign ^= 1;
         }
-        /* This is the way the PowerPC specification defines it */
-        float128 ft0_128, ft1_128;
-
-        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
-        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
-        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
-        if (unlikely(float128_is_infinity(ft0_128) &&
-                     float64_is_infinity(farg3.d) &&
-                     float128_is_neg(ft0_128) == float64_is_neg(farg3.d))) {
-            /* Magnitude subtraction of infinities */
-            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
-        } else {
-            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
-            ft0_128 = float128_sub(ft0_128, ft1_128, &env->fp_status);
-            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
+        if (aSign ^ bSign ^ cSign) {
+            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
         }
     }
-    return farg1.ll;
 }
 
-/* fnmadd - fnmadd. */
-uint64_t helper_fnmadd(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
-                       uint64_t arg3)
-{
-    CPU_DoubleU farg1, farg2, farg3;
-
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-    farg3.ll = arg3;
-
-    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
-        /* Multiplication of zero by infinity */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
-            /* sNaN operation */
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
-        }
-        /* This is the way the PowerPC specification defines it */
-        float128 ft0_128, ft1_128;
-
-        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
-        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
-        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
-        if (unlikely(float128_is_infinity(ft0_128) &&
-                     float64_is_infinity(farg3.d) &&
-                     float128_is_neg(ft0_128) != float64_is_neg(farg3.d))) {
-            /* Magnitude subtraction of infinities */
-            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
-        } else {
-            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
-            ft0_128 = float128_add(ft0_128, ft1_128, &env->fp_status);
-            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
-        }
-        if (likely(!float64_is_any_nan(farg1.d))) {
-            farg1.d = float64_chs(farg1.d);
-        }
-    }
-    return farg1.ll;
+#define FPU_FMADD(op, madd_flags)                                       \
+uint64_t helper_##op(CPUPPCState *env, uint64_t arg1,                   \
+                     uint64_t arg2, uint64_t arg3)                      \
+{                                                                       \
+    uint32_t flags;                                                     \
+    float64 ret = float64_muladd(arg1, arg2, arg3, madd_flags,          \
+                                 &env->fp_status);                      \
+    flags = get_float_exception_flags(&env->fp_status);                 \
+    if (flags) {                                                        \
+        if (flags & float_flag_invalid) {                               \
+            float64_maddsub_update_excp(env, arg1, arg2, arg3,          \
+                                        madd_flags);                    \
+        }                                                               \
+        float_check_status(env);                                        \
+    }                                                                   \
+    return ret;                                                         \
 }
 
-/* fnmsub - fnmsub. */
-uint64_t helper_fnmsub(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
-                       uint64_t arg3)
-{
-    CPU_DoubleU farg1, farg2, farg3;
-
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-    farg3.ll = arg3;
+#define MADD_FLGS 0
+#define MSUB_FLGS float_muladd_negate_c
+#define NMADD_FLGS float_muladd_negate_result
+#define NMSUB_FLGS (float_muladd_negate_c | float_muladd_negate_result)
 
-    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                 (float64_is_zero(farg1.d) &&
-                  float64_is_infinity(farg2.d)))) {
-        /* Multiplication of zero by infinity */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
-            /* sNaN operation */
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
-        }
-        /* This is the way the PowerPC specification defines it */
-        float128 ft0_128, ft1_128;
-
-        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
-        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
-        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
-        if (unlikely(float128_is_infinity(ft0_128) &&
-                     float64_is_infinity(farg3.d) &&
-                     float128_is_neg(ft0_128) == float64_is_neg(farg3.d))) {
-            /* Magnitude subtraction of infinities */
-            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
-        } else {
-            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
-            ft0_128 = float128_sub(ft0_128, ft1_128, &env->fp_status);
-            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
-        }
-        if (likely(!float64_is_any_nan(farg1.d))) {
-            farg1.d = float64_chs(farg1.d);
-        }
-    }
-    return farg1.ll;
-}
+FPU_FMADD(fmadd, MADD_FLGS)
+FPU_FMADD(fnmadd, NMADD_FLGS)
+FPU_FMADD(fmsub, MSUB_FLGS)
+FPU_FMADD(fnmsub, NMSUB_FLGS)
 
 /* frsp - frsp. */
 uint64_t helper_frsp(CPUPPCState *env, uint64_t arg)
@@ -2384,11 +2268,6 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                           \
     float_check_status(env);                                                  \
 }
 
-#define MADD_FLGS 0
-#define MSUB_FLGS float_muladd_negate_c
-#define NMADD_FLGS float_muladd_negate_result
-#define NMSUB_FLGS (float_muladd_negate_c | float_muladd_negate_result)
-
 VSX_MADD(xsmaddadp, 1, float64, VsrD(0), MADD_FLGS, 1, 1, 0)
 VSX_MADD(xsmaddmdp, 1, float64, VsrD(0), MADD_FLGS, 0, 1, 0)
 VSX_MADD(xsmsubadp, 1, float64, VsrD(0), MSUB_FLGS, 1, 1, 0)
-- 
2.7.4


Re: [Qemu-devel] [PATCH v2] target/ppc: rewrite f[n]m[add, sub] using float64_muladd
Posted by David Gibson 7 years ago
On Thu, Mar 02, 2017 at 07:40:29PM +0530, Nikunj A Dadhania wrote:
> Use the softfloat api for fused multiply-add.
> Introduce routine to set the FPSCR flags VXNAN, VXIMZ nad VMISI.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

Merged, thanks.

> 
> ---
> 
> v1:
> * Removed float64_madd_set_vxisi()
> * Introduced float64_maddsub_update_excp() will updated required flags
> * Changed layout of error handling
> 
> v0:
> * Use MADD/MSUB_FLAGS as used by VSX instructions
> * Introduce helper float64_madd_set_vxisi()
> ---
>  target/ppc/fpu_helper.c | 213 +++++++++++-------------------------------------
>  1 file changed, 46 insertions(+), 167 deletions(-)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 58aee64..0535ad0 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -743,178 +743,62 @@ uint64_t helper_frim(CPUPPCState *env, uint64_t arg)
>      return do_fri(env, arg, float_round_down);
>  }
>  
> -/* fmadd - fmadd. */
> -uint64_t helper_fmadd(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
> -                      uint64_t arg3)
> +static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
> +                                        float64 arg2, float64 arg3,
> +                                        unsigned int madd_flags)
>  {
> -    CPU_DoubleU farg1, farg2, farg3;
> -
> -    farg1.ll = arg1;
> -    farg2.ll = arg2;
> -    farg3.ll = arg3;
> -
> -    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
> -                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
> -        /* Multiplication of zero by infinity */
> -        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> -    } else {
> -        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
> -            /* sNaN operation */
> -            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> -        }
> -        /* This is the way the PowerPC specification defines it */
> -        float128 ft0_128, ft1_128;
> -
> -        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
> -        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
> -        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
> -        if (unlikely(float128_is_infinity(ft0_128) &&
> -                     float64_is_infinity(farg3.d) &&
> -                     float128_is_neg(ft0_128) != float64_is_neg(farg3.d))) {
> -            /* Magnitude subtraction of infinities */
> -            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
> -        } else {
> -            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
> -            ft0_128 = float128_add(ft0_128, ft1_128, &env->fp_status);
> -            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
> -        }
> -    }
> -
> -    return farg1.ll;
> -}
> -
> -/* fmsub - fmsub. */
> -uint64_t helper_fmsub(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
> -                      uint64_t arg3)
> -{
> -    CPU_DoubleU farg1, farg2, farg3;
> -
> -    farg1.ll = arg1;
> -    farg2.ll = arg2;
> -    farg3.ll = arg3;
> -
> -    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
> -                 (float64_is_zero(farg1.d) &&
> -                  float64_is_infinity(farg2.d)))) {
> +    if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
> +                 (float64_is_zero(arg1) && float64_is_infinity(arg2)))) {
>          /* Multiplication of zero by infinity */
> -        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> -    } else {
> -        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
> -            /* sNaN operation */
> -            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> +        arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> +    } else if (unlikely(float64_is_signaling_nan(arg1, &env->fp_status) ||
> +                        float64_is_signaling_nan(arg2, &env->fp_status) ||
> +                        float64_is_signaling_nan(arg3, &env->fp_status))) {
> +        /* sNaN operation */
> +        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> +    } else if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
> +               float64_is_infinity(arg3)) {
> +        uint8_t aSign, bSign, cSign;
> +
> +        aSign = float64_is_neg(arg1);
> +        bSign = float64_is_neg(arg2);
> +        cSign = float64_is_neg(arg3);
> +        if (madd_flags & float_muladd_negate_c) {
> +            cSign ^= 1;
>          }
> -        /* This is the way the PowerPC specification defines it */
> -        float128 ft0_128, ft1_128;
> -
> -        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
> -        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
> -        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
> -        if (unlikely(float128_is_infinity(ft0_128) &&
> -                     float64_is_infinity(farg3.d) &&
> -                     float128_is_neg(ft0_128) == float64_is_neg(farg3.d))) {
> -            /* Magnitude subtraction of infinities */
> -            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
> -        } else {
> -            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
> -            ft0_128 = float128_sub(ft0_128, ft1_128, &env->fp_status);
> -            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
> +        if (aSign ^ bSign ^ cSign) {
> +            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
>          }
>      }
> -    return farg1.ll;
>  }
>  
> -/* fnmadd - fnmadd. */
> -uint64_t helper_fnmadd(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
> -                       uint64_t arg3)
> -{
> -    CPU_DoubleU farg1, farg2, farg3;
> -
> -    farg1.ll = arg1;
> -    farg2.ll = arg2;
> -    farg3.ll = arg3;
> -
> -    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
> -                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
> -        /* Multiplication of zero by infinity */
> -        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> -    } else {
> -        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
> -            /* sNaN operation */
> -            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> -        }
> -        /* This is the way the PowerPC specification defines it */
> -        float128 ft0_128, ft1_128;
> -
> -        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
> -        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
> -        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
> -        if (unlikely(float128_is_infinity(ft0_128) &&
> -                     float64_is_infinity(farg3.d) &&
> -                     float128_is_neg(ft0_128) != float64_is_neg(farg3.d))) {
> -            /* Magnitude subtraction of infinities */
> -            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
> -        } else {
> -            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
> -            ft0_128 = float128_add(ft0_128, ft1_128, &env->fp_status);
> -            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
> -        }
> -        if (likely(!float64_is_any_nan(farg1.d))) {
> -            farg1.d = float64_chs(farg1.d);
> -        }
> -    }
> -    return farg1.ll;
> +#define FPU_FMADD(op, madd_flags)                                       \
> +uint64_t helper_##op(CPUPPCState *env, uint64_t arg1,                   \
> +                     uint64_t arg2, uint64_t arg3)                      \
> +{                                                                       \
> +    uint32_t flags;                                                     \
> +    float64 ret = float64_muladd(arg1, arg2, arg3, madd_flags,          \
> +                                 &env->fp_status);                      \
> +    flags = get_float_exception_flags(&env->fp_status);                 \
> +    if (flags) {                                                        \
> +        if (flags & float_flag_invalid) {                               \
> +            float64_maddsub_update_excp(env, arg1, arg2, arg3,          \
> +                                        madd_flags);                    \
> +        }                                                               \
> +        float_check_status(env);                                        \
> +    }                                                                   \
> +    return ret;                                                         \
>  }
>  
> -/* fnmsub - fnmsub. */
> -uint64_t helper_fnmsub(CPUPPCState *env, uint64_t arg1, uint64_t arg2,
> -                       uint64_t arg3)
> -{
> -    CPU_DoubleU farg1, farg2, farg3;
> -
> -    farg1.ll = arg1;
> -    farg2.ll = arg2;
> -    farg3.ll = arg3;
> +#define MADD_FLGS 0
> +#define MSUB_FLGS float_muladd_negate_c
> +#define NMADD_FLGS float_muladd_negate_result
> +#define NMSUB_FLGS (float_muladd_negate_c | float_muladd_negate_result)
>  
> -    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
> -                 (float64_is_zero(farg1.d) &&
> -                  float64_is_infinity(farg2.d)))) {
> -        /* Multiplication of zero by infinity */
> -        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> -    } else {
> -        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg2.d, &env->fp_status) ||
> -                     float64_is_signaling_nan(farg3.d, &env->fp_status))) {
> -            /* sNaN operation */
> -            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> -        }
> -        /* This is the way the PowerPC specification defines it */
> -        float128 ft0_128, ft1_128;
> -
> -        ft0_128 = float64_to_float128(farg1.d, &env->fp_status);
> -        ft1_128 = float64_to_float128(farg2.d, &env->fp_status);
> -        ft0_128 = float128_mul(ft0_128, ft1_128, &env->fp_status);
> -        if (unlikely(float128_is_infinity(ft0_128) &&
> -                     float64_is_infinity(farg3.d) &&
> -                     float128_is_neg(ft0_128) == float64_is_neg(farg3.d))) {
> -            /* Magnitude subtraction of infinities */
> -            farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
> -        } else {
> -            ft1_128 = float64_to_float128(farg3.d, &env->fp_status);
> -            ft0_128 = float128_sub(ft0_128, ft1_128, &env->fp_status);
> -            farg1.d = float128_to_float64(ft0_128, &env->fp_status);
> -        }
> -        if (likely(!float64_is_any_nan(farg1.d))) {
> -            farg1.d = float64_chs(farg1.d);
> -        }
> -    }
> -    return farg1.ll;
> -}
> +FPU_FMADD(fmadd, MADD_FLGS)
> +FPU_FMADD(fnmadd, NMADD_FLGS)
> +FPU_FMADD(fmsub, MSUB_FLGS)
> +FPU_FMADD(fnmsub, NMSUB_FLGS)
>  
>  /* frsp - frsp. */
>  uint64_t helper_frsp(CPUPPCState *env, uint64_t arg)
> @@ -2384,11 +2268,6 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                           \
>      float_check_status(env);                                                  \
>  }
>  
> -#define MADD_FLGS 0
> -#define MSUB_FLGS float_muladd_negate_c
> -#define NMADD_FLGS float_muladd_negate_result
> -#define NMSUB_FLGS (float_muladd_negate_c | float_muladd_negate_result)
> -
>  VSX_MADD(xsmaddadp, 1, float64, VsrD(0), MADD_FLGS, 1, 1, 0)
>  VSX_MADD(xsmaddmdp, 1, float64, VsrD(0), MADD_FLGS, 0, 1, 0)
>  VSX_MADD(xsmsubadp, 1, float64, VsrD(0), MSUB_FLGS, 1, 1, 0)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH v2] target/ppc: rewrite f[n]m[add, sub] using float64_muladd
Posted by Richard Henderson 7 years ago
On 03/03/2017 01:10 AM, Nikunj A Dadhania wrote:
> +static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
> +                                        float64 arg2, float64 arg3,
> +                                        unsigned int madd_flags)
>  {
> +    if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
> +                 (float64_is_zero(arg1) && float64_is_infinity(arg2)))) {
>          /* Multiplication of zero by infinity */
> +        arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> +    } else if (unlikely(float64_is_signaling_nan(arg1, &env->fp_status) ||
> +                        float64_is_signaling_nan(arg2, &env->fp_status) ||
> +                        float64_is_signaling_nan(arg3, &env->fp_status))) {
> +        /* sNaN operation */
> +        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);

Are you sure you shouldn't be testing for NaN first?
Do you really get VXIMZ if arg3 (the addend) is (S)NaN?

You should probably eliminate QNaN as well, before the other checks.


r~

Re: [Qemu-devel] [PATCH v2] target/ppc: rewrite f[n]m[add, sub] using float64_muladd
Posted by David Gibson 7 years ago
On Fri, Mar 03, 2017 at 02:31:38PM +1100, Richard Henderson wrote:
> On 03/03/2017 01:10 AM, Nikunj A Dadhania wrote:
> > +static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
> > +                                        float64 arg2, float64 arg3,
> > +                                        unsigned int madd_flags)
> >  {
> > +    if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
> > +                 (float64_is_zero(arg1) && float64_is_infinity(arg2)))) {
> >          /* Multiplication of zero by infinity */
> > +        arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> > +    } else if (unlikely(float64_is_signaling_nan(arg1, &env->fp_status) ||
> > +                        float64_is_signaling_nan(arg2, &env->fp_status) ||
> > +                        float64_is_signaling_nan(arg3, &env->fp_status))) {
> > +        /* sNaN operation */
> > +        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> 
> Are you sure you shouldn't be testing for NaN first?
> Do you really get VXIMZ if arg3 (the addend) is (S)NaN?
> 
> You should probably eliminate QNaN as well, before the other checks.

Oh.. I've already merged and pull requested this.  I'm afraid any
further corrections will have to be done as followup patches.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH v2] target/ppc: rewrite f[n]m[add, sub] using float64_muladd
Posted by Nikunj A Dadhania 7 years ago
David Gibson <david@gibson.dropbear.id.au> writes:

> [ Unknown signature status ]
> On Fri, Mar 03, 2017 at 02:31:38PM +1100, Richard Henderson wrote:
>> On 03/03/2017 01:10 AM, Nikunj A Dadhania wrote:
>> > +static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
>> > +                                        float64 arg2, float64 arg3,
>> > +                                        unsigned int madd_flags)
>> >  {
>> > +    if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
>> > +                 (float64_is_zero(arg1) && float64_is_infinity(arg2)))) {
>> >          /* Multiplication of zero by infinity */
>> > +        arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
>> > +    } else if (unlikely(float64_is_signaling_nan(arg1, &env->fp_status) ||
>> > +                        float64_is_signaling_nan(arg2, &env->fp_status) ||
>> > +                        float64_is_signaling_nan(arg3, &env->fp_status))) {
>> > +        /* sNaN operation */
>> > +        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
>> 
>> Are you sure you shouldn't be testing for NaN first?
>> Do you really get VXIMZ if arg3 (the addend) is (S)NaN?
>> 
>> You should probably eliminate QNaN as well, before the other checks.
>
> Oh.. I've already merged and pull requested this.

The behaviour has not changed as compared to previous version.

> I'm afraid any further corrections will have to be done as followup
> patches.

I will send a follow-up with fixing other multiply-add versions as well.

Regards
Nikunj


Re: [Qemu-devel] [PATCH v2] target/ppc: rewrite f[n]m[add, sub] using float64_muladd
Posted by Nikunj A Dadhania 7 years ago
Richard Henderson <rth@twiddle.net> writes:
> On 03/03/2017 01:10 AM, Nikunj A Dadhania wrote:
>> +static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
>> +                                        float64 arg2, float64 arg3,
>> +                                        unsigned int madd_flags)
>>  {
>> +    if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
>> +                 (float64_is_zero(arg1) && float64_is_infinity(arg2)))) {
>>          /* Multiplication of zero by infinity */
>> +        arg1 = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
>> +    } else if (unlikely(float64_is_signaling_nan(arg1, &env->fp_status) ||
>> +                        float64_is_signaling_nan(arg2, &env->fp_status) ||
>> +                        float64_is_signaling_nan(arg3, &env->fp_status))) {
>> +        /* sNaN operation */
>> +        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
>
> Are you sure you shouldn't be testing for NaN first?

I have tried to retain the old order of checking. While when I see
MultiplyAddDP(x,y,z) in ISA, following is the order:

    * If x, y or z is an SNaN, vxsnan_flag is set to 1.
    * If x is a Zero and y, is an Infinity or x is an Infinity and y is
      an Zero, vximz_flag is set to 1.
    * If the product of x and y is an Infinity and z is an Infinity of
      the opposite sign, vxisi_flag is set to 1.

In that case, I need to get rid of nested if-else and make each of them
independent checks.
    
> Do you really get VXIMZ if arg3 (the addend) is (S)NaN?

Yes, with the above logic yes. 

> You should probably eliminate QNaN as well, before the other checks.

MultiplyAddDP does not eliminate QNaN. But then, there is detailed spec
on what should be returned in various cases(Page 469 ISA 3.0)

Regards
Nikunj