[Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Aurelien Jarno]

float{32,64}_muladd takes an enum as a parameter, and not flags. It
means the parameter should be checked with == test instead of &.

Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 fpu/softfloat.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index b29256a..518e45b 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -2171,15 +2171,15 @@ float32 float32_muladd(float32 a, float32 b, float32 c, int flags STATUS_PARAM)
         return float32_default_nan;
     }
 
-    if (flags & float_muladd_negate_c) {
+    if (flags == float_muladd_negate_c) {
         cSign ^= 1;
     }
 
-    signflip = (flags & float_muladd_negate_result) ? 1 : 0;
+    signflip = (flags == float_muladd_negate_result) ? 1 : 0;
 
     /* Work out the sign and type of the product */
     pSign = aSign ^ bSign;
-    if (flags & float_muladd_negate_product) {
+    if (flags == float_muladd_negate_product) {
         pSign ^= 1;
     }
     pInf = (aExp == 0xff) || (bExp == 0xff);
@@ -3724,15 +3724,15 @@ float64 float64_muladd(float64 a, float64 b, float64 c, int flags STATUS_PARAM)
         return float64_default_nan;
     }
 
-    if (flags & float_muladd_negate_c) {
+    if (flags == float_muladd_negate_c) {
         cSign ^= 1;
     }
 
-    signflip = (flags & float_muladd_negate_result) ? 1 : 0;
+    signflip = (flags == float_muladd_negate_result) ? 1 : 0;
 
     /* Work out the sign and type of the product */
     pSign = aSign ^ bSign;
-    if (flags & float_muladd_negate_product) {
+    if (flags == float_muladd_negate_product) {
         pSign ^= 1;
     }
     pInf = (aExp == 0x7ff) || (bExp == 0x7ff);
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/5] target-ppc: simplify NaN propagation for vector functions

[Aurelien Jarno]

Commit e024e881bb1a8b5085026589360d26ed97acdd64 provided a pickNaN()
function for PowerPC, implementing the correct NaN propagation rules.
Therefore there is no need to test the operands manually, we can rely
on the softfloat code to do that.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/int_helper.c |   26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
index f638b2a..5b2a3c8 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -409,9 +409,7 @@ VARITH(uwm, u32)
         int i;                                                          \
                                                                         \
         for (i = 0; i < ARRAY_SIZE(r->f); i++) {                        \
-            HANDLE_NAN2(r->f[i], a->f[i], b->f[i]) {                    \
-                r->f[i] = func(a->f[i], b->f[i], &env->vec_status);     \
-            }                                                           \
+            r->f[i] = func(a->f[i], b->f[i], &env->vec_status);         \
         }                                                               \
     }
 VARITHFP(addfp, float32_add)
@@ -1039,9 +1037,7 @@ void helper_vrefp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *b)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(r->f); i++) {
-        HANDLE_NAN1(r->f[i], b->f[i]) {
-            r->f[i] = float32_div(float32_one, b->f[i], &env->vec_status);
-        }
+        r->f[i] = float32_div(float32_one, b->f[i], &env->vec_status);
     }
 }
 
@@ -1054,9 +1050,7 @@ void helper_vrefp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *b)
                                                                 \
         set_float_rounding_mode(rounding, &s);                  \
         for (i = 0; i < ARRAY_SIZE(r->f); i++) {                \
-            HANDLE_NAN1(r->f[i], b->f[i]) {                     \
-                r->f[i] = float32_round_to_int (b->f[i], &s);   \
-            }                                                   \
+            r->f[i] = float32_round_to_int (b->f[i], &s);       \
         }                                                       \
     }
 VRFI(n, float_round_nearest_even)
@@ -1089,11 +1083,9 @@ void helper_vrsqrtefp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *b)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(r->f); i++) {
-        HANDLE_NAN1(r->f[i], b->f[i]) {
-            float32 t = float32_sqrt(b->f[i], &env->vec_status);
+        float32 t = float32_sqrt(b->f[i], &env->vec_status);
 
-            r->f[i] = float32_div(float32_one, t, &env->vec_status);
-        }
+        r->f[i] = float32_div(float32_one, t, &env->vec_status);
     }
 }
 
@@ -1109,9 +1101,7 @@ void helper_vexptefp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *b)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(r->f); i++) {
-        HANDLE_NAN1(r->f[i], b->f[i]) {
-            r->f[i] = float32_exp2(b->f[i], &env->vec_status);
-        }
+        r->f[i] = float32_exp2(b->f[i], &env->vec_status);
     }
 }
 
@@ -1120,9 +1110,7 @@ void helper_vlogefp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *b)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(r->f); i++) {
-        HANDLE_NAN1(r->f[i], b->f[i]) {
-            r->f[i] = float32_log2(b->f[i], &env->vec_status);
-        }
+        r->f[i] = float32_log2(b->f[i], &env->vec_status);
     }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/5] target-ppc: use the softfloat min/max functions

[Aurelien Jarno]

Use the new softfloat float32_min() and float32_max() to implement the
vminfp and vmaxfp instructions. As a bonus we can get rid of the call to
the HANDLE_NAN2 macro, as the NaN handling is directly done at the
softfloat level.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/int_helper.c |   23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
index 5b2a3c8..6141243 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -414,6 +414,8 @@ VARITH(uwm, u32)
     }
 VARITHFP(addfp, float32_add)
 VARITHFP(subfp, float32_sub)
+VARITHFP(minfp, float32_min)
+VARITHFP(maxfp, float32_max)
 #undef VARITHFP
 
 #define VARITHSAT_CASE(type, op, cvt, element)                          \
@@ -728,27 +730,6 @@ VMINMAX(uw, u32)
 #undef VMINMAX_DO
 #undef VMINMAX
 
-#define VMINMAXFP(suffix, rT, rF)                                       \
-    void helper_v##suffix(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, \
-                          ppc_avr_t *b)                                 \
-    {                                                                   \
-        int i;                                                          \
-                                                                        \
-        for (i = 0; i < ARRAY_SIZE(r->f); i++) {                        \
-            HANDLE_NAN2(r->f[i], a->f[i], b->f[i]) {                    \
-                if (float32_lt_quiet(a->f[i], b->f[i],                  \
-                                     &env->vec_status)) {               \
-                    r->f[i] = rT->f[i];                                 \
-                } else {                                                \
-                    r->f[i] = rF->f[i];                                 \
-                }                                                       \
-            }                                                           \
-        }                                                               \
-    }
-VMINMAXFP(minfp, a, b)
-VMINMAXFP(maxfp, b, a)
-#undef VMINMAXFP
-
 void helper_vmladduhm(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b, ppc_avr_t *c)
 {
     int i;
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/5] target-ppc: use the softfloat float32_muladd function

[Aurelien Jarno]

Use the new softfloat float32_muladd() function to implement the vmaddfp
and vnmsubfp instructions. As a bonus we can get rid of the call to the
HANDLE_NAN3 macro, as the NaN handling is directly done at the softfloat
level.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/int_helper.c |   57 ++++++++++++-----------------------------------
 1 file changed, 14 insertions(+), 43 deletions(-)

diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
index 6141243..51cb97c 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -418,6 +418,20 @@ VARITHFP(minfp, float32_min)
 VARITHFP(maxfp, float32_max)
 #undef VARITHFP
 
+#define VARITHFPFMA(suffix, type)                                       \
+    void helper_v##suffix(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, \
+                           ppc_avr_t *b, ppc_avr_t *c)                  \
+    {                                                                   \
+        int i;                                                          \
+        for (i = 0; i < ARRAY_SIZE(r->f); i++) {                        \
+            r->f[i] = float32_muladd(a->f[i], c->f[i], b->f[i],         \
+                                     type, &env->vec_status);           \
+        }                                                               \
+    }
+VARITHFPFMA(maddfp, 0);
+VARITHFPFMA(nmsubfp, float_muladd_negate_result);
+#undef VARITHFPFMA
+
 #define VARITHSAT_CASE(type, op, cvt, element)                          \
     {                                                                   \
         type result = (type)a->element[i] op (type)b->element[i];       \
@@ -649,27 +663,6 @@ VCT(uxs, cvtsduw, u32)
 VCT(sxs, cvtsdsw, s32)
 #undef VCT
 
-void helper_vmaddfp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b,
-                    ppc_avr_t *c)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(r->f); i++) {
-        HANDLE_NAN3(r->f[i], a->f[i], b->f[i], c->f[i]) {
-            /* Need to do the computation in higher precision and round
-             * once at the end.  */
-            float64 af, bf, cf, t;
-
-            af = float32_to_float64(a->f[i], &env->vec_status);
-            bf = float32_to_float64(b->f[i], &env->vec_status);
-            cf = float32_to_float64(c->f[i], &env->vec_status);
-            t = float64_mul(af, cf, &env->vec_status);
-            t = float64_add(t, bf, &env->vec_status);
-            r->f[i] = float64_to_float32(t, &env->vec_status);
-        }
-    }
-}
-
 void helper_vmhaddshs(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
                       ppc_avr_t *b, ppc_avr_t *c)
 {
@@ -909,28 +902,6 @@ VMUL(uh, u16, u32)
 #undef VMUL_DO
 #undef VMUL
 
-void helper_vnmsubfp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
-                     ppc_avr_t *b, ppc_avr_t *c)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(r->f); i++) {
-        HANDLE_NAN3(r->f[i], a->f[i], b->f[i], c->f[i]) {
-            /* Need to do the computation is higher precision and round
-             * once at the end.  */
-            float64 af, bf, cf, t;
-
-            af = float32_to_float64(a->f[i], &env->vec_status);
-            bf = float32_to_float64(b->f[i], &env->vec_status);
-            cf = float32_to_float64(c->f[i], &env->vec_status);
-            t = float64_mul(af, cf, &env->vec_status);
-            t = float64_sub(t, bf, &env->vec_status);
-            t = float64_chs(t);
-            r->f[i] = float64_to_float32(t, &env->vec_status);
-        }
-    }
-}
-
 void helper_vperm(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b,
                   ppc_avr_t *c)
 {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/5] target-ppc: use the softfloat float32_muladd function

[Peter Maydell]

On 8 September 2012 22:12, Aurelien Jarno <aurelien@aurel32.net> wrote:
> +#define VARITHFPFMA(suffix, type)                                       \
> +    void helper_v##suffix(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, \
> +                           ppc_avr_t *b, ppc_avr_t *c)                  \
> +    {                                                                   \
> +        int i;                                                          \
> +        for (i = 0; i < ARRAY_SIZE(r->f); i++) {                        \
> +            r->f[i] = float32_muladd(a->f[i], c->f[i], b->f[i],         \
> +                                     type, &env->vec_status);           \
> +        }                                                               \
> +    }
> +VARITHFPFMA(maddfp, 0);
> +VARITHFPFMA(nmsubfp, float_muladd_negate_result);
> +#undef VARITHFPFMA
> +
>  #define VARITHSAT_CASE(type, op, cvt, element)                          \
>      {                                                                   \
>          type result = (type)a->element[i] op (type)b->element[i];       \
> -void helper_vnmsubfp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
> -                     ppc_avr_t *b, ppc_avr_t *c)
> -{
> -    int i;
> -
> -    for (i = 0; i < ARRAY_SIZE(r->f); i++) {
> -        HANDLE_NAN3(r->f[i], a->f[i], b->f[i], c->f[i]) {
> -            /* Need to do the computation is higher precision and round
> -             * once at the end.  */
> -            float64 af, bf, cf, t;
> -
> -            af = float32_to_float64(a->f[i], &env->vec_status);
> -            bf = float32_to_float64(b->f[i], &env->vec_status);
> -            cf = float32_to_float64(c->f[i], &env->vec_status);
> -            t = float64_mul(af, cf, &env->vec_status);
> -            t = float64_sub(t, bf, &env->vec_status);
> -            t = float64_chs(t);
> -            r->f[i] = float64_to_float32(t, &env->vec_status);
> -        }
> -    }
> -}

I mentioned this in my comment on the other patch, but just to attach
it to the right patch for the benefit of the archives:
the code here for vnmsub is (correctly) doing a subtraction of bf
and then negating the final result, so you need to pass float_muladd
the flags negate_result | negate_c, not just negate_result.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/5] target-ppc: use the softfloat float32_muladd function

[Aurelien Jarno]

On Sun, Sep 09, 2012 at 10:51:20AM +0100, Peter Maydell wrote:
> On 8 September 2012 22:12, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > +#define VARITHFPFMA(suffix, type)                                       \
> > +    void helper_v##suffix(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a, \
> > +                           ppc_avr_t *b, ppc_avr_t *c)                  \
> > +    {                                                                   \
> > +        int i;                                                          \
> > +        for (i = 0; i < ARRAY_SIZE(r->f); i++) {                        \
> > +            r->f[i] = float32_muladd(a->f[i], c->f[i], b->f[i],         \
> > +                                     type, &env->vec_status);           \
> > +        }                                                               \
> > +    }
> > +VARITHFPFMA(maddfp, 0);
> > +VARITHFPFMA(nmsubfp, float_muladd_negate_result);
> > +#undef VARITHFPFMA
> > +
> >  #define VARITHSAT_CASE(type, op, cvt, element)                          \
> >      {                                                                   \
> >          type result = (type)a->element[i] op (type)b->element[i];       \
> > -void helper_vnmsubfp(CPUPPCState *env, ppc_avr_t *r, ppc_avr_t *a,
> > -                     ppc_avr_t *b, ppc_avr_t *c)
> > -{
> > -    int i;
> > -
> > -    for (i = 0; i < ARRAY_SIZE(r->f); i++) {
> > -        HANDLE_NAN3(r->f[i], a->f[i], b->f[i], c->f[i]) {
> > -            /* Need to do the computation is higher precision and round
> > -             * once at the end.  */
> > -            float64 af, bf, cf, t;
> > -
> > -            af = float32_to_float64(a->f[i], &env->vec_status);
> > -            bf = float32_to_float64(b->f[i], &env->vec_status);
> > -            cf = float32_to_float64(c->f[i], &env->vec_status);
> > -            t = float64_mul(af, cf, &env->vec_status);
> > -            t = float64_sub(t, bf, &env->vec_status);
> > -            t = float64_chs(t);
> > -            r->f[i] = float64_to_float32(t, &env->vec_status);
> > -        }
> > -    }
> > -}
> 
> I mentioned this in my comment on the other patch, but just to attach
> it to the right patch for the benefit of the archives:
> the code here for vnmsub is (correctly) doing a subtraction of bf
> and then negating the final result, so you need to pass float_muladd
> the flags negate_result | negate_c, not just negate_result.
> 

Correct, or alternatively it could use negate_product. I'll send an
updated patch later.


-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH 5/5] target-ppc: get rid of the HANDLE_NAN{1, 2, 3} macros

[Aurelien Jarno]

We can finally get rid of the ugly HANDLE_NAN{1,2,3} macros.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/int_helper.c |   21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c
index 51cb97c..6d8bf4d 100644
--- a/target-ppc/int_helper.c
+++ b/target-ppc/int_helper.c
@@ -287,23 +287,6 @@ target_ulong helper_602_mfrom(target_ulong arg)
     for (index = ARRAY_SIZE(r->element)-1; index >= 0; index--)
 #endif
 
-/* If X is a NaN, store the corresponding QNaN into RESULT.  Otherwise,
- * execute the following block.  */
-#define DO_HANDLE_NAN(result, x)                        \
-    if (float32_is_any_nan(x)) {                        \
-        CPU_FloatU __f;                                 \
-        __f.f = x;                                      \
-        __f.l = __f.l | (1 << 22);  /* Set QNaN bit. */ \
-        result = __f.f;                                 \
-    } else
-
-#define HANDLE_NAN1(result, x)                  \
-    DO_HANDLE_NAN(result, x)
-#define HANDLE_NAN2(result, x, y)                       \
-    DO_HANDLE_NAN(result, x) DO_HANDLE_NAN(result, y)
-#define HANDLE_NAN3(result, x, y, z)                                    \
-    DO_HANDLE_NAN(result, x) DO_HANDLE_NAN(result, y) DO_HANDLE_NAN(result, z)
-
 /* Saturating arithmetic helpers.  */
 #define SATCVT(from, to, from_type, to_type, min, max)          \
     static inline to_type cvt##from##to(from_type x, int *sat)  \
@@ -1413,10 +1396,6 @@ VUPK(lsh, s32, s16, UPKLO)
 #undef UPKHI
 #undef UPKLO
 
-#undef DO_HANDLE_NAN
-#undef HANDLE_NAN1
-#undef HANDLE_NAN2
-#undef HANDLE_NAN3
 #undef VECTOR_FOR_INORDER_I
 #undef HI_IDX
 #undef LO_IDX
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 5/5] target-ppc: get rid of the HANDLE_NAN{1, 2, 3} macros

[Peter Maydell]

On 8 September 2012 22:12, Aurelien Jarno <aurelien@aurel32.net> wrote:
> We can finally get rid of the ugly HANDLE_NAN{1,2,3} macros.

This is nice. I feel like it justifies my adding all that stuff
to fpu/ for ARM :-)

-- PMM

Re: [Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Max Filippov]

On Sun, Sep 9, 2012 at 1:12 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> float{32,64}_muladd takes an enum as a parameter, and not flags. It
> means the parameter should be checked with == test instead of &.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---

Hi Aurelien,

I've also stumbled upon this bug, have a patch for it in the xtensa tree.
I guess that the interface was designed to allow combining these flags, why
don't just make them independent:

diff --git a/fpu/softfloat.h b/fpu/softfloat.h
index feec3a1..2860ca0 100644
--- a/fpu/softfloat.h
+++ b/fpu/softfloat.h
@@ -219,7 +219,7 @@ void float_raise( int8 flags STATUS_PARAM);
 enum {
     float_muladd_negate_c = 1,
     float_muladd_negate_product = 2,
-    float_muladd_negate_result = 3,
+    float_muladd_negate_result = 4,
 };

 /*----------------------------------------------------------------------------

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Peter Maydell]

On 8 September 2012 22:40, Max Filippov <jcmvbkbc@gmail.com> wrote:
> On Sun, Sep 9, 2012 at 1:12 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> float{32,64}_muladd takes an enum as a parameter, and not flags. It
>> means the parameter should be checked with == test instead of &.
>
> I've also stumbled upon this bug, have a patch for it in the xtensa tree.
> I guess that the interface was designed to allow combining these flags, why
> don't just make them independent:

Yes, the intent is that all these negations can be controlled
separately so that you can have operations which do more than
one of them. ARM doesn't use the negation options so this isn't
a "live" bug, but IIRC I did check various other architectures
and those do require various combinations of these negate flags.
So I think Max's patch is the correct one.

-- PMM

Re: [Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Aurelien Jarno]

On Sun, Sep 09, 2012 at 01:40:52AM +0400, Max Filippov wrote:
> On Sun, Sep 9, 2012 at 1:12 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > float{32,64}_muladd takes an enum as a parameter, and not flags. It
> > means the parameter should be checked with == test instead of &.
> >
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> 
> Hi Aurelien,
> 
> I've also stumbled upon this bug, have a patch for it in the xtensa tree.
> I guess that the interface was designed to allow combining these flags, why
> don't just make them independent:

My idea was that negating both the product and c is equivalent to
negating the result, so there is no need to allow all of them
independent.

> diff --git a/fpu/softfloat.h b/fpu/softfloat.h
> index feec3a1..2860ca0 100644
> --- a/fpu/softfloat.h
> +++ b/fpu/softfloat.h
> @@ -219,7 +219,7 @@ void float_raise( int8 flags STATUS_PARAM);
>  enum {
>      float_muladd_negate_c = 1,
>      float_muladd_negate_product = 2,
> -    float_muladd_negate_result = 3,
> +    float_muladd_negate_result = 4,
>  };
> 
>  /*----------------------------------------------------------------------------

That said your solution is also fine, it just allow multiple way to do
the same thing. 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Peter Maydell]

On 9 September 2012 10:24, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Sun, Sep 09, 2012 at 01:40:52AM +0400, Max Filippov wrote:
>> I've also stumbled upon this bug, have a patch for it in the xtensa tree.
>> I guess that the interface was designed to allow combining these flags, why
>> don't just make them independent:
>
> My idea was that negating both the product and c is equivalent to
> negating the result, so there is no need to allow all of them
> independent.

But at least some of them can reasonably be independent, so
at that point you might as well make them all independent flags.

In fact my reading of the PPC ISA is that it needs several combinations:
 fmadd : no flags
 fnmadd : negate_result
 fmsub : negate_c
 fnmsub : negate_result | negate_c

...and it looks like your patch is incorrectly only setting negate_result
for the fnmsub case. (I can't see anything handling the fnmadd and fmsub
insns, do we not implement them?)

(negate_product will be used by x86 and SPARC, among others.)

thanks
\x10-- PMM

Re: [Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Aurelien Jarno]

On Sun, Sep 09, 2012 at 10:47:52AM +0100, Peter Maydell wrote:
> On 9 September 2012 10:24, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Sun, Sep 09, 2012 at 01:40:52AM +0400, Max Filippov wrote:
> >> I've also stumbled upon this bug, have a patch for it in the xtensa tree.
> >> I guess that the interface was designed to allow combining these flags, why
> >> don't just make them independent:
> >
> > My idea was that negating both the product and c is equivalent to
> > negating the result, so there is no need to allow all of them
> > independent.
> 
> But at least some of them can reasonably be independent, so
> at that point you might as well make them all independent flags.
> 
> In fact my reading of the PPC ISA is that it needs several combinations:
>  fmadd : no flags
>  fnmadd : negate_result
>  fmsub : negate_c
>  fnmsub : negate_result | negate_c
> 
> ...and it looks like your patch is incorrectly only setting negate_result
> for the fnmsub case. (I can't see anything handling the fnmadd and fmsub
> insns, do we not implement them?)

fnmadd and fmsub are provided as part as floating point instructions
(something else to clean), not as part as altivec instructions.

> (negate_product will be used by x86 and SPARC, among others.)
> 
> thanks
> \x10-- PMM
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/5] softfloat: fix float{32, 64}_muladd options

[Peter Maydell]

On 9 September 2012 10:24, Aurelien Jarno <aurelien@aurel32.net> wrote:
> My idea was that negating both the product and c is equivalent to
> negating the result, so there is no need to allow all of them
> independent.

This is the kind of statement about floating point arithmetic that
sounds plausible but turns out to be untrue for some odd corner
case. Consider a = +0, b = +0, c = -0.

If we're negating the result, then:
 -((a * b) + c) = -((+0 * +0) + -0)
                = -(+0 + -0)
                = -(+0)
                = -0

(assuming round to nearest; rounding mode affects the answer
to '+0 + -0')

If we negate both product and c:
 (-(a * b) + -c) = (-(+0 * +0) + -(-0))
                 = (-(+0) + +0)
                 = (-0 + +0)
                 = +0

so the two calculations give us zeros of different sign.

-- PMM