diff mbox

[PULL,06/37] target-ppc: VXSQRT Should Not Be Set for NaNs

Message ID 1420644048-16919-7-git-send-email-agraf@suse.de
State New
Headers show

Commit Message

Alexander Graf Jan. 7, 2015, 3:20 p.m. UTC 
From: Tom Musta <tommusta@gmail.com>

The Power ISA square root instructions (fsqrt[s], frsqrte[s]) must
set the FPSCR[VXSQRT] flag when operating on a negative value.
However, NaNs have no sign and therefore this flag should not
be set when operating on one.

Change the order of the checks in the helper code.  Move the
SNaN-to-QNaN macro to the top of the file so that it can be
re-used.

Signed-off-by: Tom Musta <tommusta@gmail.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/fpu_helper.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Comments

Maciej W. Rozycki Feb. 12, 2015, 10:21 p.m. UTC | #1 
On Wed, 7 Jan 2015, Alexander Graf wrote:

> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> index 7f74466..81db60f 100644
> --- a/target-ppc/fpu_helper.c
> +++ b/target-ppc/fpu_helper.c
> @@ -920,14 +923,16 @@ uint64_t helper_fsqrt(CPUPPCState *env, uint64_t arg)
>  
>      farg.ll = arg;
>  
> -    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> -    } else {
> +    if (unlikely(float64_is_any_nan(farg.d))) {
>          if (unlikely(float64_is_signaling_nan(farg.d))) {
> -            /* sNaN square root */
> +            /* sNaN reciprocal square root */

 This change to the comment looks accidental, compare the changes below.  
Should it be reverted?  [Found this while resolving merge conflicts.]

>              fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> +            farg.ll = float64_snan_to_qnan(farg.ll);
>          }
> +    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> +    } else {
>          farg.d = float64_sqrt(farg.d, &env->fp_status);
>      }
>      return farg.ll;
> @@ -974,17 +979,20 @@ uint64_t helper_frsqrte(CPUPPCState *env, uint64_t arg)
>  
>      farg.ll = arg;
>  
> -    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> -    } else {
> +    if (unlikely(float64_is_any_nan(farg.d))) {
>          if (unlikely(float64_is_signaling_nan(farg.d))) {
>              /* sNaN reciprocal square root */
>              fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> +            farg.ll = float64_snan_to_qnan(farg.ll);
>          }
> +    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> +    } else {
>          farg.d = float64_sqrt(farg.d, &env->fp_status);
>          farg.d = float64_div(float64_one, farg.d, &env->fp_status);
>      }
> +
>      return farg.ll;
>  }
>  

  Maciej
Tom Musta Feb. 13, 2015, 2:28 p.m. UTC | #2 
I agree that the comment is incorrect and should say "sNaN square root".

On Thu, Feb 12, 2015 at 4:21 PM, Maciej W. Rozycki <macro@linux-mips.org>
wrote:

> On Wed, 7 Jan 2015, Alexander Graf wrote:
>
> > diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> > index 7f74466..81db60f 100644
> > --- a/target-ppc/fpu_helper.c
> > +++ b/target-ppc/fpu_helper.c
> > @@ -920,14 +923,16 @@ uint64_t helper_fsqrt(CPUPPCState *env, uint64_t
> arg)
> >
> >      farg.ll = arg;
> >
> > -    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> > -    } else {
> > +    if (unlikely(float64_is_any_nan(farg.d))) {
> >          if (unlikely(float64_is_signaling_nan(farg.d))) {
> > -            /* sNaN square root */
> > +            /* sNaN reciprocal square root */
>
>  This change to the comment looks accidental, compare the changes below.
> Should it be reverted?  [Found this while resolving merge conflicts.]
>
> >              fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> > +            farg.ll = float64_snan_to_qnan(farg.ll);
> >          }
> > +    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> > +    } else {
> >          farg.d = float64_sqrt(farg.d, &env->fp_status);
> >      }
> >      return farg.ll;
> > @@ -974,17 +979,20 @@ uint64_t helper_frsqrte(CPUPPCState *env, uint64_t
> arg)
> >
> >      farg.ll = arg;
> >
> > -    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> > -    } else {
> > +    if (unlikely(float64_is_any_nan(farg.d))) {
> >          if (unlikely(float64_is_signaling_nan(farg.d))) {
> >              /* sNaN reciprocal square root */
> >              fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
> > +            farg.ll = float64_snan_to_qnan(farg.ll);
> >          }
> > +    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
> > +    } else {
> >          farg.d = float64_sqrt(farg.d, &env->fp_status);
> >          farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> >      }
> > +
> >      return farg.ll;
> >  }
> >
>
>   Maciej
>
Alexander Graf Feb. 13, 2015, 11:35 p.m. UTC | #3 
On 12.02.15 23:21, Maciej W. Rozycki wrote:
> On Wed, 7 Jan 2015, Alexander Graf wrote:
> 
>> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
>> index 7f74466..81db60f 100644
>> --- a/target-ppc/fpu_helper.c
>> +++ b/target-ppc/fpu_helper.c
>> @@ -920,14 +923,16 @@ uint64_t helper_fsqrt(CPUPPCState *env, uint64_t arg)
>>  
>>      farg.ll = arg;
>>  
>> -    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
>> -    } else {
>> +    if (unlikely(float64_is_any_nan(farg.d))) {
>>          if (unlikely(float64_is_signaling_nan(farg.d))) {
>> -            /* sNaN square root */
>> +            /* sNaN reciprocal square root */
> 
>  This change to the comment looks accidental, compare the changes below.  
> Should it be reverted?  [Found this while resolving merge conflicts.]

Nicely spotted. Could you please cook up a small patch fixing it?


Thanks!

Alex
diff mbox

Patch

diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
index 7f74466..81db60f 100644
--- a/target-ppc/fpu_helper.c
+++ b/target-ppc/fpu_helper.c
@@ -19,6 +19,9 @@ 
 #include "cpu.h"
 #include "exec/helper-proto.h"
 
+#define float64_snan_to_qnan(x) ((x) | 0x0008000000000000ULL)
+#define float32_snan_to_qnan(x) ((x) | 0x00400000)
+
 /*****************************************************************************/
 /* Floating point operations helpers */
 uint64_t helper_float32_to_float64(CPUPPCState *env, uint32_t arg)
@@ -920,14 +923,16 @@  uint64_t helper_fsqrt(CPUPPCState *env, uint64_t arg)
 
     farg.ll = arg;
 
-    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
-    } else {
+    if (unlikely(float64_is_any_nan(farg.d))) {
         if (unlikely(float64_is_signaling_nan(farg.d))) {
-            /* sNaN square root */
+            /* sNaN reciprocal square root */
             fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            farg.ll = float64_snan_to_qnan(farg.ll);
         }
+    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
+    } else {
         farg.d = float64_sqrt(farg.d, &env->fp_status);
     }
     return farg.ll;
@@ -974,17 +979,20 @@  uint64_t helper_frsqrte(CPUPPCState *env, uint64_t arg)
 
     farg.ll = arg;
 
-    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(env, POWERPC_EXCP_FP_VXSQRT, 1);
-    } else {
+    if (unlikely(float64_is_any_nan(farg.d))) {
         if (unlikely(float64_is_signaling_nan(farg.d))) {
             /* sNaN reciprocal square root */
             fload_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            farg.ll = float64_snan_to_qnan(farg.ll);
         }
+    } else 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(env, POWERPC_EXCP_FP_VXSQRT, 1);
+    } else {
         farg.d = float64_sqrt(farg.d, &env->fp_status);
         farg.d = float64_div(float64_one, farg.d, &env->fp_status);
     }
+
     return farg.ll;
 }
 
@@ -2382,9 +2390,6 @@  void helper_##op(CPUPPCState *env, uint32_t opcode)                      \
 VSX_SCALAR_CMP(xscmpodp, 1)
 VSX_SCALAR_CMP(xscmpudp, 0)
 
-#define float64_snan_to_qnan(x) ((x) | 0x0008000000000000ULL)
-#define float32_snan_to_qnan(x) ((x) | 0x00400000)
-
 /* VSX_MAX_MIN - VSX floating point maximum/minimum
  *   name  - instruction mnemonic
  *   op    - operation (max or min)