[Qemu-devel] [PATCH 0/3] target/ppc: floating point multiply-add fixes

[Qemu-devel] [PATCH 0/3] target/ppc: floating point multiply-add fixes

[Nikunj A Dadhania]

Exception handling in fmadd/fmsub/fnmadd/fnmsub isnt correct as the 
order of checking could give wrong settings in FPSCR.

For example, (x * y) + z, if x = infinity, y = zero and z = snan. 
After the execution of instruction VXNAN and VXIMZ both should be set. For 
this correct the ordering in the float64_maddsub_update_excp() as follows:

      * 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.

Moreover, all vector multiply-add/substract and vector scalar multiply-add/sub 
had the bug. VXISI should be set only when (∞ - ∞), where as the instruction was
doing it wrong, was just checking ((a == ∞ OR b == ∞) && (c == ∞)). There are two 
issues here:

1. infinity can be +ve or -ve, i.e.  (∞ + (-∞)), should result in setting VXISI
2. Need to take care of the operation (add or sub). (∞ + ∞) should not set VXISI

Patch:
01: Fixes the order of checking and makes them independent as per 
    the ISA

02: Introduces the macro to be used by Vector Scalar and Vector Multiply-Add

03: Make Vector Scalar and Vector Multiply Add use the macro for updating 
    exception flags 

Nikunj A Dadhania (3):
  target/ppc: fmadd check for excp independently
  target/ppc: fmadd: add macro for updating flags
  target/ppc: use helper for excp handling

 target/ppc/fpu_helper.c | 78 +++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] target/ppc: fmadd check for excp independently

[Nikunj A Dadhania]

Current order of checking does not confirm with the spec
(ISA 3.0: MultiplyAddDP page-469). Change the order and make them
independent of each other.

For example: a = infinity, b = zero, c = SNaN, this should set both
VXIMZ and VXNAN

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 target/ppc/fpu_helper.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 0535ad0..a547f58 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -747,17 +747,21 @@ static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
                                         float64 arg2, float64 arg3,
                                         unsigned int madd_flags)
 {
+    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);
+    }
+
     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);
-    } else if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
-               float64_is_infinity(arg3)) {
+        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
+    }
+
+    if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
+        float64_is_infinity(arg3)) {
         uint8_t aSign, bSign, cSign;
 
         aSign = float64_is_neg(arg1);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] target/ppc: fmadd check for excp independently

[Richard Henderson]

On 03/03/2017 05:58 PM, Nikunj A Dadhania wrote:
> Current order of checking does not confirm with the spec
> (ISA 3.0: MultiplyAddDP page-469). Change the order and make them
> independent of each other.
>
> For example: a = infinity, b = zero, c = SNaN, this should set both
> VXIMZ and VXNAN
>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  target/ppc/fpu_helper.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 0535ad0..a547f58 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -747,17 +747,21 @@ static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
>                                          float64 arg2, float64 arg3,
>                                          unsigned int madd_flags)
>  {
> +    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);
> +    }
> +
>      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);
> -    } else if ((float64_is_infinity(arg1) || float64_is_infinity(arg2)) &&
> -               float64_is_infinity(arg3)) {
> +        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
> +    }
> +

While these two bits should be both be set if the inputs demand, it won't 
happen if the exception enable bit is set, since the first call will longjmp. 
I'm not sure what to do about that; perhaps just ignore it for now.

More importantly, it will longjmp with the wrong source location.  Note that 
float_invalid_op_excp is attempting to use __always_inline__ to grab a correct 
value for GETPC.  Which, as one can predict, is doomed to failure.  As here we 
are, in another function which is not __always_inline__, producing the wrong value.

We need to drop all of the __always_inline__ nonsense, and properly pass down a 
value for GETPC from the top-level helper, so that we always think about it.


r~

[Qemu-devel] [PATCH 2/3] target/ppc: fmadd: add macro for updating flags

[Nikunj A Dadhania]

Adds FPU_MADDSUB_UPDATE macro, this will be used for other routines
having float32/16

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 target/ppc/fpu_helper.c | 62 ++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index a547f58..56dfa18 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -743,38 +743,38 @@ uint64_t helper_frim(CPUPPCState *env, uint64_t arg)
     return do_fri(env, arg, float_round_down);
 }
 
-static void float64_maddsub_update_excp(CPUPPCState *env, float64 arg1,
-                                        float64 arg2, float64 arg3,
-                                        unsigned int madd_flags)
-{
-    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);
-    }
-
-    if (unlikely((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
-                 (float64_is_zero(arg1) && float64_is_infinity(arg2)))) {
-        /* Multiplication of zero by infinity */
-        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
-    }
-
-    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;
-        }
-        if (aSign ^ bSign ^ cSign) {
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
-        }
-    }
+#define FPU_MADDSUB_UPDATE(name, tp)                                    \
+static void name(CPUPPCState *env, float64 arg1,                        \
+                 float64 arg2, float64 arg3,                            \
+                 unsigned int madd_flags)                               \
+{                                                                       \
+    if (tp##_is_signaling_nan(arg1, &env->fp_status) ||                 \
+        tp##_is_signaling_nan(arg2, &env->fp_status) ||                 \
+        tp##_is_signaling_nan(arg3, &env->fp_status)) {                 \
+        /* sNaN operation */                                            \
+        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);          \
+    }                                                                   \
+    if ((tp##_is_infinity(arg1) && tp##_is_zero(arg2)) ||               \
+        (tp##_is_zero(arg1) && tp##_is_infinity(arg2))) {               \
+        /* Multiplication of zero by infinity */                        \
+        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);           \
+    }                                                                   \
+    if ((tp##_is_infinity(arg1) || tp##_is_infinity(arg2)) &&           \
+        tp##_is_infinity(arg3)) {                                       \
+        uint8_t aSign, bSign, cSign;                                    \
+                                                                        \
+        aSign = tp##_is_neg(arg1);                                      \
+        bSign = tp##_is_neg(arg2);                                      \
+        cSign = tp##_is_neg(arg3);                                      \
+        if (madd_flags & float_muladd_negate_c) {                       \
+            cSign ^= 1;                                                 \
+        }                                                               \
+        if (aSign ^ bSign ^ cSign) {                                    \
+            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);       \
+        }                                                               \
+    }                                                                   \
 }
+FPU_MADDSUB_UPDATE(float64_maddsub_update_excp, float64)
 
 #define FPU_FMADD(op, madd_flags)                                       \
 uint64_t helper_##op(CPUPPCState *env, uint64_t arg1,                   \
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] target/ppc: fmadd: add macro for updating flags

[Richard Henderson]

On 03/03/2017 05:58 PM, Nikunj A Dadhania wrote:
> +#define FPU_MADDSUB_UPDATE(name, tp)                                    \
> +static void name(CPUPPCState *env, float64 arg1,                        \
> +                 float64 arg2, float64 arg3,                            \
> +                 unsigned int madd_flags)                               \

Use ALL CAPS for macro arguments which you paste.
You forgot to use TP for arg{1,2,3}.


r~

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] target/ppc: fmadd: add macro for updating flags

[Nikunj Dadhania]

On 4 March 2017 at 00:45, Richard Henderson <rth@twiddle.net> wrote:
> On 03/03/2017 05:58 PM, Nikunj A Dadhania wrote:
>>
>> +#define FPU_MADDSUB_UPDATE(name, tp)                                    \
>> +static void name(CPUPPCState *env, float64 arg1,                        \
>> +                 float64 arg2, float64 arg3,                            \
>> +                 unsigned int madd_flags)                               \
>
>
> Use ALL CAPS for macro arguments which you paste.

Sure.

> You forgot to use TP for arg{1,2,3}.

Right, will correct it.

Regards
Nikunj

[Qemu-devel] [PATCH 3/3] target/ppc: use helper for excp handling

[Nikunj A Dadhania]

Use the helper routine float[32,64]_maddsub_update_excp() in VSX_MADD
macro.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 target/ppc/fpu_helper.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 56dfa18..e88071b 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -774,6 +774,7 @@ static void name(CPUPPCState *env, float64 arg1,                        \
         }                                                               \
     }                                                                   \
 }
+FPU_MADDSUB_UPDATE(float32_maddsub_update_excp, float32)
 FPU_MADDSUB_UPDATE(float64_maddsub_update_excp, float64)
 
 #define FPU_FMADD(op, madd_flags)                                       \
@@ -2240,24 +2241,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                           \
         env->fp_status.float_exception_flags |= tstat.float_exception_flags;  \
                                                                               \
         if (unlikely(tstat.float_exception_flags & float_flag_invalid)) {     \
-            if (tp##_is_signaling_nan(xa.fld, &tstat) ||                      \
-                tp##_is_signaling_nan(b->fld, &tstat) ||                      \
-                tp##_is_signaling_nan(c->fld, &tstat)) {                      \
-                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, sfprf);    \
-                tstat.float_exception_flags &= ~float_flag_invalid;           \
-            }                                                                 \
-            if ((tp##_is_infinity(xa.fld) && tp##_is_zero(b->fld)) ||         \
-                (tp##_is_zero(xa.fld) && tp##_is_infinity(b->fld))) {         \
-                xt_out.fld = float64_to_##tp(float_invalid_op_excp(env,       \
-                    POWERPC_EXCP_FP_VXIMZ, sfprf), &env->fp_status);          \
-                tstat.float_exception_flags &= ~float_flag_invalid;           \
-            }                                                                 \
-            if ((tstat.float_exception_flags & float_flag_invalid) &&         \
-                ((tp##_is_infinity(xa.fld) ||                                 \
-                  tp##_is_infinity(b->fld)) &&                                \
-                  tp##_is_infinity(c->fld))) {                                \
-                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, sfprf);     \
-            }                                                                 \
+            tp##_maddsub_update_excp(env, xa.fld, b->fld, c->fld, maddflgs);  \
         }                                                                     \
                                                                               \
         if (r2sp) {                                                           \
-- 
2.7.4