[Qemu-devel] [PATCH 0/2] linux-user: fix incorrect NaN detection in ARM nwfpe emulation

[Qemu-devel] [PATCH 0/2] linux-user: fix incorrect NaN detection in ARM nwfpe emulation

[Peter Maydell]

The code in the linux-user ARM nwfpe emulation was incorrectly checking only
for quiet NaNs when it should have been checking for any kind of NaN.  This
is probably because the code in question was taken from the Linux kernel,
whose copy of the softfloat library had been modified so that
float*_is_nan() returned true for all NaNs, not just quiet ones.  The qemu
equivalent function is float*_is_any_nan(), so use that.  NB that this code
is really obsolete since nobody uses FPE for actual arithmetic now; this is
just cleanup following the recent renaming of the NaN related functions.
(As such I have checked it against the equivalent Linux kernel code but
I don't have a test case for it.)

Peter Maydell (2):
  softfloat: Implement floatx80_is_any_nan() and float128_is_any_nan()
  linux-user: Fix incorrect NaN detection in ARM nwfpe emulation

 fpu/softfloat.h                   |   11 +++++++++++
 linux-user/arm/nwfpe/fpa11_cprt.c |   14 +++++++-------
 2 files changed, 18 insertions(+), 7 deletions(-)

[Qemu-devel] [PATCH 1/2] softfloat: Implement floatx80_is_any_nan() and float128_is_any_nan()

[Peter Maydell]

Implement versions of float*_is_any_nan() for the floatx80 and
float128 types.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 fpu/softfloat.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fpu/softfloat.h b/fpu/softfloat.h
index f2104c6..ac81845 100644
--- a/fpu/softfloat.h
+++ b/fpu/softfloat.h
@@ -469,6 +469,11 @@ INLINE int floatx80_is_zero(floatx80 a)
     return (a.high & 0x7fff) == 0 && a.low == 0;
 }
 
+INLINE int floatx80_is_any_nan(floatx80 a)
+{
+    return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
+}
+
 #endif
 
 #ifdef FLOAT128
@@ -536,6 +541,12 @@ INLINE int float128_is_zero(float128 a)
     return (a.high & 0x7fffffffffffffffLL) == 0 && a.low == 0;
 }
 
+INLINE int float128_is_any_nan(float128 a)
+{
+    return ((a.high >> 48) & 0x7fff) == 0x7fff &&
+        ((a.low != 0) || ((a.high & 0xffffffffffffLL) != 0));
+}
+
 #endif
 
 #else /* CONFIG_SOFTFLOAT */
-- 
1.6.3.3

[Qemu-devel] [PATCH 2/2] linux-user: Fix incorrect NaN detection in ARM nwfpe emulation

[Peter Maydell]

The code in the linux-user ARM nwfpe emulation was incorrectly
checking only for quiet NaNs when it should have been checking
for any kind of NaN. This is probably because the code in
question was taken from the Linux kernel, whose copy of the
softfloat library had been modified so that float*_is_nan()
returned true for all NaNs, not just quiet ones. The qemu
equivalent function is float*_is_any_nan(), so use that.
NB that this code is really obsolete since nobody uses FPE
for actual arithmetic now; this is just cleanup following
the recent renaming of the NaN related functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/arm/nwfpe/fpa11_cprt.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/linux-user/arm/nwfpe/fpa11_cprt.c b/linux-user/arm/nwfpe/fpa11_cprt.c
index 0e61b58..be54e95 100644
--- a/linux-user/arm/nwfpe/fpa11_cprt.c
+++ b/linux-user/arm/nwfpe/fpa11_cprt.c
@@ -199,21 +199,21 @@ static unsigned int PerformComparison(const unsigned int opcode)
    {
       case typeSingle:
         //printk("single.\n");
-	if (float32_is_quiet_nan(fpa11->fpreg[Fn].fSingle))
+	if (float32_is_any_nan(fpa11->fpreg[Fn].fSingle))
 	   goto unordered;
         rFn = float32_to_floatx80(fpa11->fpreg[Fn].fSingle, &fpa11->fp_status);
       break;
 
       case typeDouble:
         //printk("double.\n");
-	if (float64_is_quiet_nan(fpa11->fpreg[Fn].fDouble))
+	if (float64_is_any_nan(fpa11->fpreg[Fn].fDouble))
 	   goto unordered;
         rFn = float64_to_floatx80(fpa11->fpreg[Fn].fDouble, &fpa11->fp_status);
       break;
 
       case typeExtended:
         //printk("extended.\n");
-	if (floatx80_is_quiet_nan(fpa11->fpreg[Fn].fExtended))
+	if (floatx80_is_any_nan(fpa11->fpreg[Fn].fExtended))
 	   goto unordered;
         rFn = fpa11->fpreg[Fn].fExtended;
       break;
@@ -225,7 +225,7 @@ static unsigned int PerformComparison(const unsigned int opcode)
    {
      //printk("Fm is a constant: #%d.\n",Fm);
      rFm = getExtendedConstant(Fm);
-     if (floatx80_is_quiet_nan(rFm))
+     if (floatx80_is_any_nan(rFm))
         goto unordered;
    }
    else
@@ -235,21 +235,21 @@ static unsigned int PerformComparison(const unsigned int opcode)
       {
          case typeSingle:
            //printk("single.\n");
-	   if (float32_is_quiet_nan(fpa11->fpreg[Fm].fSingle))
+	   if (float32_is_any_nan(fpa11->fpreg[Fm].fSingle))
 	      goto unordered;
            rFm = float32_to_floatx80(fpa11->fpreg[Fm].fSingle, &fpa11->fp_status);
          break;
 
          case typeDouble:
            //printk("double.\n");
-	   if (float64_is_quiet_nan(fpa11->fpreg[Fm].fDouble))
+	   if (float64_is_any_nan(fpa11->fpreg[Fm].fDouble))
 	      goto unordered;
            rFm = float64_to_floatx80(fpa11->fpreg[Fm].fDouble, &fpa11->fp_status);
          break;
 
          case typeExtended:
            //printk("extended.\n");
-	   if (floatx80_is_quiet_nan(fpa11->fpreg[Fm].fExtended))
+	   if (floatx80_is_any_nan(fpa11->fpreg[Fm].fExtended))
 	      goto unordered;
            rFm = fpa11->fpreg[Fm].fExtended;
          break;
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH 1/2] softfloat: Implement floatx80_is_any_nan() and float128_is_any_nan()

[Aurelien Jarno]

On Thu, Jan 06, 2011 at 06:34:43PM +0000, Peter Maydell wrote:
> Implement versions of float*_is_any_nan() for the floatx80 and
> float128 types.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  fpu/softfloat.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fpu/softfloat.h b/fpu/softfloat.h
> index f2104c6..ac81845 100644
> --- a/fpu/softfloat.h
> +++ b/fpu/softfloat.h
> @@ -469,6 +469,11 @@ INLINE int floatx80_is_zero(floatx80 a)
>      return (a.high & 0x7fff) == 0 && a.low == 0;
>  }
>  
> +INLINE int floatx80_is_any_nan(floatx80 a)
> +{
> +    return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
> +}
> +
>  #endif

While this looks correct, this seems to say that our definition of
floatx80_is_quiet_nan() (for SNAN_BIT_IS_ZERO) is wrong as it is exactly
the same.

>  #ifdef FLOAT128
> @@ -536,6 +541,12 @@ INLINE int float128_is_zero(float128 a)
>      return (a.high & 0x7fffffffffffffffLL) == 0 && a.low == 0;
>  }
>  
> +INLINE int float128_is_any_nan(float128 a)
> +{
> +    return ((a.high >> 48) & 0x7fff) == 0x7fff &&
> +        ((a.low != 0) || ((a.high & 0xffffffffffffLL) != 0));
> +}
> +
>  #endif
>  
>  #else /* CONFIG_SOFTFLOAT */
> -- 
> 1.6.3.3
> 

Acked-by: Aurelien Jarno <aurelien@aurel32.net>

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

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix incorrect NaN detection in ARM nwfpe emulation

[Aurelien Jarno]

On Thu, Jan 06, 2011 at 06:34:44PM +0000, Peter Maydell wrote:
> The code in the linux-user ARM nwfpe emulation was incorrectly
> checking only for quiet NaNs when it should have been checking
> for any kind of NaN. This is probably because the code in
> question was taken from the Linux kernel, whose copy of the
> softfloat library had been modified so that float*_is_nan()
> returned true for all NaNs, not just quiet ones. The qemu
> equivalent function is float*_is_any_nan(), so use that.
> NB that this code is really obsolete since nobody uses FPE
> for actual arithmetic now; this is just cleanup following
> the recent renaming of the NaN related functions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/arm/nwfpe/fpa11_cprt.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)

Acked-by: Aurelien Jarno <aurelien@aurel32.net>

> diff --git a/linux-user/arm/nwfpe/fpa11_cprt.c b/linux-user/arm/nwfpe/fpa11_cprt.c
> index 0e61b58..be54e95 100644
> --- a/linux-user/arm/nwfpe/fpa11_cprt.c
> +++ b/linux-user/arm/nwfpe/fpa11_cprt.c
> @@ -199,21 +199,21 @@ static unsigned int PerformComparison(const unsigned int opcode)
>     {
>        case typeSingle:
>          //printk("single.\n");
> -	if (float32_is_quiet_nan(fpa11->fpreg[Fn].fSingle))
> +	if (float32_is_any_nan(fpa11->fpreg[Fn].fSingle))
>  	   goto unordered;
>          rFn = float32_to_floatx80(fpa11->fpreg[Fn].fSingle, &fpa11->fp_status);
>        break;
>  
>        case typeDouble:
>          //printk("double.\n");
> -	if (float64_is_quiet_nan(fpa11->fpreg[Fn].fDouble))
> +	if (float64_is_any_nan(fpa11->fpreg[Fn].fDouble))
>  	   goto unordered;
>          rFn = float64_to_floatx80(fpa11->fpreg[Fn].fDouble, &fpa11->fp_status);
>        break;
>  
>        case typeExtended:
>          //printk("extended.\n");
> -	if (floatx80_is_quiet_nan(fpa11->fpreg[Fn].fExtended))
> +	if (floatx80_is_any_nan(fpa11->fpreg[Fn].fExtended))
>  	   goto unordered;
>          rFn = fpa11->fpreg[Fn].fExtended;
>        break;
> @@ -225,7 +225,7 @@ static unsigned int PerformComparison(const unsigned int opcode)
>     {
>       //printk("Fm is a constant: #%d.\n",Fm);
>       rFm = getExtendedConstant(Fm);
> -     if (floatx80_is_quiet_nan(rFm))
> +     if (floatx80_is_any_nan(rFm))
>          goto unordered;
>     }
>     else
> @@ -235,21 +235,21 @@ static unsigned int PerformComparison(const unsigned int opcode)
>        {
>           case typeSingle:
>             //printk("single.\n");
> -	   if (float32_is_quiet_nan(fpa11->fpreg[Fm].fSingle))
> +	   if (float32_is_any_nan(fpa11->fpreg[Fm].fSingle))
>  	      goto unordered;
>             rFm = float32_to_floatx80(fpa11->fpreg[Fm].fSingle, &fpa11->fp_status);
>           break;
>  
>           case typeDouble:
>             //printk("double.\n");
> -	   if (float64_is_quiet_nan(fpa11->fpreg[Fm].fDouble))
> +	   if (float64_is_any_nan(fpa11->fpreg[Fm].fDouble))
>  	      goto unordered;
>             rFm = float64_to_floatx80(fpa11->fpreg[Fm].fDouble, &fpa11->fp_status);
>           break;
>  
>           case typeExtended:
>             //printk("extended.\n");
> -	   if (floatx80_is_quiet_nan(fpa11->fpreg[Fm].fExtended))
> +	   if (floatx80_is_any_nan(fpa11->fpreg[Fm].fExtended))
>  	      goto unordered;
>             rFm = fpa11->fpreg[Fm].fExtended;
>           break;
> -- 
> 1.6.3.3
> 
> 
> 

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

Re: [Qemu-devel] [PATCH 1/2] softfloat: Implement floatx80_is_any_nan() and float128_is_any_nan()

[Peter Maydell]

On 7 January 2011 15:28, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Thu, Jan 06, 2011 at 06:34:43PM +0000, Peter Maydell wrote:
>> Implement versions of float*_is_any_nan() for the floatx80 and
>> float128 types.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  fpu/softfloat.h |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/fpu/softfloat.h b/fpu/softfloat.h
>> index f2104c6..ac81845 100644
>> --- a/fpu/softfloat.h
>> +++ b/fpu/softfloat.h
>> @@ -469,6 +469,11 @@ INLINE int floatx80_is_zero(floatx80 a)
>>      return (a.high & 0x7fff) == 0 && a.low == 0;
>>  }
>>
>> +INLINE int floatx80_is_any_nan(floatx80 a)
>> +{
>> +    return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
>> +}
>> +
>>  #endif
>
> While this looks correct, this seems to say that our definition of
> floatx80_is_quiet_nan() (for SNAN_BIT_IS_ZERO) is wrong as it is exactly
> the same.

Hrm. I suspect this is confusion caused by floatx80 having
an explicit hidden bit (most significant bit of the significand)
where float32/float64 have an implicit hidden bit. I think
floatx80_is_quiet_nan() must be wrong because:

int floatx80_is_quiet_nan( floatx80 a )
{
#if SNAN_BIT_IS_ONE
    bits64 aLow;

    aLow = a.low & ~ LIT64( 0x4000000000000000 );
    return
           ( ( a.high & 0x7FFF ) == 0x7FFF )
        && (bits64) ( aLow<<1 )
        && ( a.low == aLow );
#else
    return ( ( a.high & 0x7FFF ) == 0x7FFF ) && (bits64) ( a.low<<1 );
#endif
}

the two halves of the ifdef ought to carve the space up
into two disjoint halves, but you can see that the
!SNAN_BIT_IS_ONE condition is a superset of the other.

-- PMM