diff mbox

[3/3] target-ppc: fix sNaN propagation

Message ID 1293979183-27108-4-git-send-email-aurelien@aurel32.net
State New
Headers show

Commit Message

Aurelien Jarno Jan. 2, 2011, 2:39 p.m. UTC 
The current FPU code returns 0.0 if one of the operand is a
signaling NaN and the VXSNAN exception is disabled.

fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
exception as the operand should be propagated instead of a new
qNaN to be generated. Fix that by calling fload_invalid_op_excp()
only for the exception generation (if enabled), and use the softfloat
code to correctly compute the result.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-ppc/op_helper.c |  145 +++++++++++++++++++++++++----------------------
 1 files changed, 77 insertions(+), 68 deletions(-)

Comments

Alexander Graf Jan. 5, 2011, 5:20 p.m. UTC | #1 
On 02.01.2011, at 15:39, Aurelien Jarno wrote:

> The current FPU code returns 0.0 if one of the operand is a
> signaling NaN and the VXSNAN exception is disabled.
> 
> fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> exception as the operand should be propagated instead of a new
> qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> only for the exception generation (if enabled), and use the softfloat
> code to correctly compute the result.

Reading through this I'm afraid I understand too little of the matter. Anyone else who's more proficient in FP feels like taking up the review?


Alex
Aurelien Jarno Jan. 10, 2011, 7:26 p.m. UTC | #2 
On Wed, Jan 05, 2011 at 06:20:07PM +0100, Alexander Graf wrote:
> 
> On 02.01.2011, at 15:39, Aurelien Jarno wrote:
> 
> > The current FPU code returns 0.0 if one of the operand is a
> > signaling NaN and the VXSNAN exception is disabled.
> > 
> > fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> > exception as the operand should be propagated instead of a new
> > qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> > only for the exception generation (if enabled), and use the softfloat
> > code to correctly compute the result.
> 
> Reading through this I'm afraid I understand too little of the matter. Anyone else who's more proficient in FP feels like taking up the review?
> 

Anybody to review this code? Nathan maybe?
Peter Maydell Jan. 11, 2011, 12:14 a.m. UTC | #3 
On 2 January 2011 08:39, Aurelien Jarno <aurelien@aurel32.net> wrote:
> The current FPU code returns 0.0 if one of the operand is a
> signaling NaN and the VXSNAN exception is disabled.
>
> fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> exception as the operand should be propagated instead of a new
> qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> only for the exception generation (if enabled), and use the softfloat
> code to correctly compute the result.
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

> @@ -1410,10 +1418,10 @@ uint64_t helper_frsp (uint64_t arg)
>     if (unlikely(float64_is_signaling_nan(farg.d))) {
>         /* sNaN square root */
>        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> -    } else {
> -       f32 = float64_to_float32(farg.d, &env->fp_status);
> -       farg.d = float32_to_float64(f32, &env->fp_status);
>     }
> +    f32 = float64_to_float32(farg.d, &env->fp_status);
> +    farg.d = float32_to_float64(f32, &env->fp_status);
> +
>     return farg.ll;
>  }

Most of these changes are to ignoring the result from
fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN),
but this one leaves it assigning a value to farg.ll -- is there
any reason for that? (It looks like the assignment gets
immediately overwritten by the assignment to farg.d later.)

> @@ -1460,11 +1468,11 @@ uint64_t helper_fres (uint64_t arg)
>     if (unlikely(float64_is_signaling_nan(farg.d))) {
>         /* sNaN reciprocal */
>         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> -    } else {
> -        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> -        f32 = float64_to_float32(farg.d, &env->fp_status);
> -        farg.d = float32_to_float64(f32, &env->fp_status);
>     }
> +    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> +    f32 = float64_to_float32(farg.d, &env->fp_status);
> +    farg.d = float32_to_float64(f32, &env->fp_status);
> +
>     return farg.ll;
>  }
>

Ditto for this hunk.

Looks plausible to me other than that.

-- PMM
Aurelien Jarno Jan. 11, 2011, 6:23 a.m. UTC | #4 
On Mon, Jan 10, 2011 at 06:14:18PM -0600, Peter Maydell wrote:
> On 2 January 2011 08:39, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > The current FPU code returns 0.0 if one of the operand is a
> > signaling NaN and the VXSNAN exception is disabled.
> >
> > fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> > exception as the operand should be propagated instead of a new
> > qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> > only for the exception generation (if enabled), and use the softfloat
> > code to correctly compute the result.
> >
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> > @@ -1410,10 +1418,10 @@ uint64_t helper_frsp (uint64_t arg)
> >     if (unlikely(float64_is_signaling_nan(farg.d))) {
> >         /* sNaN square root */
> >        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > -    } else {
> > -       f32 = float64_to_float32(farg.d, &env->fp_status);
> > -       farg.d = float32_to_float64(f32, &env->fp_status);
> >     }
> > +    f32 = float64_to_float32(farg.d, &env->fp_status);
> > +    farg.d = float32_to_float64(f32, &env->fp_status);
> > +
> >     return farg.ll;
> >  }
> 
> Most of these changes are to ignoring the result from
> fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN),
> but this one leaves it assigning a value to farg.ll -- is there
> any reason for that? (It looks like the assignment gets
> immediately overwritten by the assignment to farg.d later.)

It is actually a mistake, though the result is correct. I'll resend the
patch after fixing that, thanks for the review.

> > @@ -1460,11 +1468,11 @@ uint64_t helper_fres (uint64_t arg)
> >     if (unlikely(float64_is_signaling_nan(farg.d))) {
> >         /* sNaN reciprocal */
> >         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > -    } else {
> > -        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> > -        f32 = float64_to_float32(farg.d, &env->fp_status);
> > -        farg.d = float32_to_float64(f32, &env->fp_status);
> >     }
> > +    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> > +    f32 = float64_to_float32(farg.d, &env->fp_status);
> > +    farg.d = float32_to_float64(f32, &env->fp_status);
> > +
> >     return farg.ll;
> >  }
> >
> 
> Ditto for this hunk.
> 
> Looks plausible to me other than that.
> 

Same here.
diff mbox

Patch

diff --git a/target-ppc/op_helper.c b/target-ppc/op_helper.c
index 279f345..ea030d0 100644
--- a/target-ppc/op_helper.c
+++ b/target-ppc/op_helper.c
@@ -975,15 +975,16 @@  uint64_t helper_fadd (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN addition */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
-                      float64_is_neg(farg1.d) != float64_is_neg(farg2.d))) {
+    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
+                 float64_is_neg(farg1.d) != float64_is_neg(farg2.d))) {
         /* Magnitude subtraction of infinities */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXISI);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN addition */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg1.d = float64_add(farg1.d, farg2.d, &env->fp_status);
     }
 
@@ -998,15 +999,16 @@  uint64_t helper_fsub (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN subtraction */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
-                      float64_is_neg(farg1.d) == float64_is_neg(farg2.d))) {
+    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
+                 float64_is_neg(farg1.d) == float64_is_neg(farg2.d))) {
         /* Magnitude subtraction of infinities */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXISI);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN subtraction */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg1.d = float64_sub(farg1.d, farg2.d, &env->fp_status);
     }
 
@@ -1021,16 +1023,17 @@  uint64_t helper_fmul (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN multiplication */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else 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(farg1.d) && float64_is_zero(farg2.d)) ||
+                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
-        farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN multiplication */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
+        float64_mul(farg1.d, farg2.d, &env->fp_status);
     }
 
     return farg1.ll;
@@ -1044,17 +1047,18 @@  uint64_t helper_fdiv (uint64_t arg1, uint64_t arg2)
     farg1.ll = arg1;
     farg2.ll = arg2;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d))) {
-        /* sNaN division */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d))) {
+    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d))) {
         /* Division of infinity by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIDI);
     } else if (unlikely(float64_is_zero(farg1.d) && float64_is_zero(farg2.d))) {
         /* Division of zero by zero */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXZDZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d))) {
+            /* sNaN division */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
     }
 
@@ -1232,16 +1236,17 @@  uint64_t helper_fmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else 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(farg1.d) && float64_is_zero(farg2.d)) ||
+                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1276,16 +1281,17 @@  uint64_t helper_fmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
+    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 = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1319,16 +1325,17 @@  uint64_t helper_fnmadd (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else 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(farg1.d) && float64_is_zero(farg2.d)) ||
+                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
         /* Multiplication of zero by infinity */
         farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1364,16 +1371,17 @@  uint64_t helper_fnmsub (uint64_t arg1, uint64_t arg2, uint64_t arg3)
     farg2.ll = arg2;
     farg3.ll = arg3;
 
-    if (unlikely(float64_is_signaling_nan(farg1.d) ||
-                 float64_is_signaling_nan(farg2.d) ||
-                 float64_is_signaling_nan(farg3.d))) {
-        /* sNaN operation */
-        farg1.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
+    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 = fload_invalid_op_excp(POWERPC_EXCP_FP_VXIMZ);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg1.d) ||
+                     float64_is_signaling_nan(farg2.d) ||
+                     float64_is_signaling_nan(farg3.d))) {
+            /* sNaN operation */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
 #ifdef FLOAT128
         /* This is the way the PowerPC specification defines it */
         float128 ft0_128, ft1_128;
@@ -1410,10 +1418,10 @@  uint64_t helper_frsp (uint64_t arg)
     if (unlikely(float64_is_signaling_nan(farg.d))) {
         /* sNaN square root */
        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else {
-       f32 = float64_to_float32(farg.d, &env->fp_status);
-       farg.d = float32_to_float64(f32, &env->fp_status);
     }
+    f32 = float64_to_float32(farg.d, &env->fp_status);
+    farg.d = float32_to_float64(f32, &env->fp_status);
+
     return farg.ll;
 }
 
@@ -1423,13 +1431,14 @@  uint64_t helper_fsqrt (uint64_t arg)
     CPU_DoubleU farg;
     farg.ll = arg;
 
-    if (unlikely(float64_is_signaling_nan(farg.d))) {
-        /* sNaN square root */
-        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
+    if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
         /* Square root of a negative nonzero number */
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSQRT);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg.d))) {
+            /* sNaN square root */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg.d = float64_sqrt(farg.d, &env->fp_status);
     }
     return farg.ll;
@@ -1443,10 +1452,9 @@  uint64_t helper_fre (uint64_t arg)
 
     if (unlikely(float64_is_signaling_nan(farg.d))) {
         /* sNaN reciprocal */
-        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else {
-        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
+        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
     }
+    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
     return farg.d;
 }
 
@@ -1460,11 +1468,11 @@  uint64_t helper_fres (uint64_t arg)
     if (unlikely(float64_is_signaling_nan(farg.d))) {
         /* sNaN reciprocal */
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else {
-        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
-        f32 = float64_to_float32(farg.d, &env->fp_status);
-        farg.d = float32_to_float64(f32, &env->fp_status);
     }
+    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
+    f32 = float64_to_float32(farg.d, &env->fp_status);
+    farg.d = float32_to_float64(f32, &env->fp_status);
+
     return farg.ll;
 }
 
@@ -1475,13 +1483,14 @@  uint64_t helper_frsqrte (uint64_t arg)
     float32 f32;
     farg.ll = arg;
 
-    if (unlikely(float64_is_signaling_nan(farg.d))) {
-        /* sNaN reciprocal square root */
-        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
-    } else if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
+    if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
         /* Reciprocal square root of a negative nonzero number */
         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSQRT);
     } else {
+        if (unlikely(float64_is_signaling_nan(farg.d))) {
+            /* sNaN reciprocal square root */
+            fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
+        }
         farg.d = float64_sqrt(farg.d, &env->fp_status);
         farg.d = float64_div(float64_one, farg.d, &env->fp_status);
         f32 = float64_to_float32(farg.d, &env->fp_status);