[Qemu-trivial] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Qemu-trivial] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Thomas Schwinge]

Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 fpu/softfloat-specialize.h |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git fpu/softfloat-specialize.h fpu/softfloat-specialize.h
index 518f694..83add1a 100644
--- fpu/softfloat-specialize.h
+++ fpu/softfloat-specialize.h
@@ -934,15 +934,14 @@ static commonNaNT floatx80ToCommonNaN( floatx80 a STATUS_PARAM)
     commonNaNT z;
 
     if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
-    if ( a.low >> 63 ) {
-        z.sign = a.high >> 15;
-        z.low = 0;
-        z.high = a.low << 1;
-    } else {
-        z.sign = floatx80_default_nan_high >> 15;
-        z.low = 0;
-        z.high = floatx80_default_nan_low << 1;
+    /* Replace a Pseudo NaN with a default NaN.  */
+    if (!(a.low >> 63)) {
+        a.low = floatx80_default_nan_low;
+        a.high = floatx80_default_nan_high;
     }
+    z.sign = a.high >> 15;
+    z.low = 0;
+    z.high = a.low << 1;
     return z;
 }
 
-- 
1.7.10.4

Re: [Qemu-trivial] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Michael Tokarev]

31.05.2013 13:39, Thomas Schwinge wrote:
> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
> ---
>  fpu/softfloat-specialize.h |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git fpu/softfloat-specialize.h fpu/softfloat-specialize.h
> index 518f694..83add1a 100644
> --- fpu/softfloat-specialize.h
> +++ fpu/softfloat-specialize.h
> @@ -934,15 +934,14 @@ static commonNaNT floatx80ToCommonNaN( floatx80 a STATUS_PARAM)
>      commonNaNT z;
>  
>      if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
> -    if ( a.low >> 63 ) {
> -        z.sign = a.high >> 15;
> -        z.low = 0;
> -        z.high = a.low << 1;
> -    } else {
> -        z.sign = floatx80_default_nan_high >> 15;
> -        z.low = 0;
> -        z.high = floatx80_default_nan_low << 1;
> +    /* Replace a Pseudo NaN with a default NaN.  */
> +    if (!(a.low >> 63)) {
> +        a.low = floatx80_default_nan_low;
> +        a.high = floatx80_default_nan_high;
>      }
> +    z.sign = a.high >> 15;
> +    z.low = 0;
> +    z.high = a.low << 1;
>      return z;
>  }

Hmm. And where's the simplification?  Here's context diff for the same:

*** fpu/softfloat-specialize.h.orig	2013-05-31 16:02:51.614710351 +0400
--- fpu/softfloat-specialize.h	2013-05-31 16:02:59.838820308 +0400
***************
*** 936,946 ****
      if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
!     if ( a.low >> 63 ) {
!         z.sign = a.high >> 15;
!         z.low = 0;
!         z.high = a.low << 1;
!     } else {
!         z.sign = floatx80_default_nan_high >> 15;
!         z.low = 0;
!         z.high = floatx80_default_nan_low << 1;
      }
      return z;
--- 936,945 ----
      if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
!     /* Replace a Pseudo NaN with a default NaN.  */
!     if (!(a.low >> 63)) {
!         a.low = floatx80_default_nan_low;
!         a.high = floatx80_default_nan_high;
      }
+     z.sign = a.high >> 15;
+     z.low = 0;
+     z.high = a.low << 1;
      return z;


Yes, your version is 3 lines shorter, because it
does not have extra else{} (2 lines) and the
remaining if() construct is one line shorter too,
due to moving z.low=0 construct into common place.

But I don't think your version is more readable, --
before it was easy to understand what is going on,
we had two easy case with all right stuff done for
each case.  Now we do some preparation before, so
the common case works.

Generated code should be about the same anyway, but
to me (IMHO!), original code is a bit more readable.

Thanks,

/mjt

Re: [Qemu-trivial] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Paolo Bonzini]

Il 31/05/2013 14:07, Michael Tokarev ha scritto:
> 31.05.2013 13:39, Thomas Schwinge wrote:
>> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
>> ---
>>  fpu/softfloat-specialize.h |   15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git fpu/softfloat-specialize.h fpu/softfloat-specialize.h
>> index 518f694..83add1a 100644
>> --- fpu/softfloat-specialize.h
>> +++ fpu/softfloat-specialize.h
>> @@ -934,15 +934,14 @@ static commonNaNT floatx80ToCommonNaN( floatx80 a STATUS_PARAM)
>>      commonNaNT z;
>>  
>>      if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
>> -    if ( a.low >> 63 ) {
>> -        z.sign = a.high >> 15;
>> -        z.low = 0;
>> -        z.high = a.low << 1;
>> -    } else {
>> -        z.sign = floatx80_default_nan_high >> 15;
>> -        z.low = 0;
>> -        z.high = floatx80_default_nan_low << 1;
>> +    /* Replace a Pseudo NaN with a default NaN.  */
>> +    if (!(a.low >> 63)) {
>> +        a.low = floatx80_default_nan_low;
>> +        a.high = floatx80_default_nan_high;
>>      }
>> +    z.sign = a.high >> 15;
>> +    z.low = 0;
>> +    z.high = a.low << 1;
>>      return z;
>>  }
> 
> Hmm. And where's the simplification?  Here's context diff for the same:
> 
> *** fpu/softfloat-specialize.h.orig	2013-05-31 16:02:51.614710351 +0400
> --- fpu/softfloat-specialize.h	2013-05-31 16:02:59.838820308 +0400
> ***************
> *** 936,946 ****
>       if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
> !     if ( a.low >> 63 ) {
> !         z.sign = a.high >> 15;
> !         z.low = 0;
> !         z.high = a.low << 1;
> !     } else {
> !         z.sign = floatx80_default_nan_high >> 15;
> !         z.low = 0;
> !         z.high = floatx80_default_nan_low << 1;
>       }
>       return z;
> --- 936,945 ----
>       if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
> !     /* Replace a Pseudo NaN with a default NaN.  */
> !     if (!(a.low >> 63)) {
> !         a.low = floatx80_default_nan_low;
> !         a.high = floatx80_default_nan_high;
>       }
> +     z.sign = a.high >> 15;
> +     z.low = 0;
> +     z.high = a.low << 1;
>       return z;
> 
> 
> Yes, your version is 3 lines shorter, because it
> does not have extra else{} (2 lines) and the
> remaining if() construct is one line shorter too,
> due to moving z.low=0 construct into common place.
> 
> But I don't think your version is more readable, --
> before it was easy to understand what is going on,
> we had two easy case with all right stuff done for
> each case.  Now we do some preparation before, so
> the common case works.
> 
> Generated code should be about the same anyway, but
> to me (IMHO!), original code is a bit more readable.

I agree.  It's also not trivial.

Paolo

Re: [Qemu-trivial] [Qemu-devel] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Peter Maydell]

On 31 May 2013 13:07, Michael Tokarev <mjt@tls.msk.ru> wrote:
> Hmm. And where's the simplification?  Here's context diff for the same:
>
> *** fpu/softfloat-specialize.h.orig     2013-05-31 16:02:51.614710351 +0400
> --- fpu/softfloat-specialize.h  2013-05-31 16:02:59.838820308 +0400
> ***************
> *** 936,946 ****
>       if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
> !     if ( a.low >> 63 ) {
> !         z.sign = a.high >> 15;
> !         z.low = 0;
> !         z.high = a.low << 1;
> !     } else {
> !         z.sign = floatx80_default_nan_high >> 15;
> !         z.low = 0;
> !         z.high = floatx80_default_nan_low << 1;
>       }
>       return z;
> --- 936,945 ----
>       if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
> !     /* Replace a Pseudo NaN with a default NaN.  */
> !     if (!(a.low >> 63)) {
> !         a.low = floatx80_default_nan_low;
> !         a.high = floatx80_default_nan_high;
>       }
> +     z.sign = a.high >> 15;
> +     z.low = 0;
> +     z.high = a.low << 1;
>       return z;
>
>
> Yes, your version is 3 lines shorter, because it
> does not have extra else{} (2 lines) and the
> remaining if() construct is one line shorter too,
> due to moving z.low=0 construct into common place.
>
> But I don't think your version is more readable, --
> before it was easy to understand what is going on,
> we had two easy case with all right stuff done for
> each case.  Now we do some preparation before, so
> the common case works.

I think you could make a reasonable argument for this
change being an improvement, because it makes it clear
what we're doing: if what we have is an x86 pseudo-NaN,
we replace it with the 80-bit default NaN, and then
proceed to do 80-bit-to-canonical conversion in the
usual way. The comment also explains why this if()
exists for the 80 bit case when it doesn't for the
equivalent 32 bit and 64 bit functions. As a code
change I actually quite like it.

> Generated code should be about the same anyway

NaN handling is well out of the fast path, so this
isn't particularly important.

That said, I think any new patches to fpu/ need to
come with an explicit statement that they can be
licensed under the softfloat-2a license or GPLv2
or BSD (etc etc) so they aren't an obstacle to
the softfloat-2a-to-2b conversion that is in the works.
[cc'd Anthony so he can correct me if I'm wrong.]

thanks
-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 3073 bytes --]

Hi!

On Fri, 31 May 2013 13:34:12 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 May 2013 13:07, Michael Tokarev <mjt@tls.msk.ru> wrote:
> > Hmm. And where's the simplification?  Here's context diff for the same:
> >
> > *** fpu/softfloat-specialize.h.orig     2013-05-31 16:02:51.614710351 +0400
> > --- fpu/softfloat-specialize.h  2013-05-31 16:02:59.838820308 +0400
> > ***************
> > *** 936,946 ****
> >       if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
> > !     if ( a.low >> 63 ) {
> > !         z.sign = a.high >> 15;
> > !         z.low = 0;
> > !         z.high = a.low << 1;
> > !     } else {
> > !         z.sign = floatx80_default_nan_high >> 15;
> > !         z.low = 0;
> > !         z.high = floatx80_default_nan_low << 1;
> >       }
> >       return z;
> > --- 936,945 ----
> >       if ( floatx80_is_signaling_nan( a ) ) float_raise( float_flag_invalid STATUS_VAR);
> > !     /* Replace a Pseudo NaN with a default NaN.  */
> > !     if (!(a.low >> 63)) {
> > !         a.low = floatx80_default_nan_low;
> > !         a.high = floatx80_default_nan_high;
> >       }
> > +     z.sign = a.high >> 15;
> > +     z.low = 0;
> > +     z.high = a.low << 1;
> >       return z;
> >
> >
> > Yes, your version is 3 lines shorter, because it
> > does not have extra else{} (2 lines) and the
> > remaining if() construct is one line shorter too,
> > due to moving z.low=0 construct into common place.
> >
> > But I don't think your version is more readable, --
> > before it was easy to understand what is going on,
> > we had two easy case with all right stuff done for
> > each case.  Now we do some preparation before, so
> > the common case works.
> 
> I think you could make a reasonable argument for this
> change being an improvement, because it makes it clear
> what we're doing: if what we have is an x86 pseudo-NaN,
> we replace it with the 80-bit default NaN, and then
> proceed to do 80-bit-to-canonical conversion in the
> usual way. The comment also explains why this if()
> exists for the 80 bit case when it doesn't for the
> equivalent 32 bit and 64 bit functions. As a code
> change I actually quite like it.

Yes, this exactly is my reasoning: first, convert a x86 Pseudo NaN into a
generic NaN, then do the floating-point type conversion itself).  I
thought this would be obvious (and hence the patch trivial) -- hey, I
even added a comment! -- but apparently what is obvious/trivial to one
isn't to the other.  :-)


> That said, I think any new patches to fpu/ need to
> come with an explicit statement that they can be
> licensed under the softfloat-2a license or GPLv2
> or BSD (etc etc) so they aren't an obstacle to
> the softfloat-2a-to-2b conversion that is in the works.
> [cc'd Anthony so he can correct me if I'm wrong.]

I hereby place this one contribution (which I think wouldn't constitute a
copyrightable change anyway) into the Public Domain, allowing any kind of
usage.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [Qemu-trivial] [Qemu-devel] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Peter Maydell]

On 31 May 2013 14:01, Thomas Schwinge <thomas@codesourcery.com> wrote:
> On Fri, 31 May 2013 13:34:12 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
>> That said, I think any new patches to fpu/ need to
>> come with an explicit statement that they can be
>> licensed under the softfloat-2a license or GPLv2
>> or BSD (etc etc) so they aren't an obstacle to
>> the softfloat-2a-to-2b conversion that is in the works.
>> [cc'd Anthony so he can correct me if I'm wrong.]
>
> I hereby place this one contribution (which I think wouldn't constitute a
> copyrightable change anyway) into the Public Domain, allowing any kind of
> usage.

I think we'd generally suggest creative commons CC0 as being
in less of a legally grey area internationally than "public
domain", if you're happy with that.

Either way,

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH] fpu: Simplify floatx80ToCommonNaN function.

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1445 bytes --]

Hi!

On Fri, 31 May 2013 15:45:55 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 May 2013 14:01, Thomas Schwinge <thomas@codesourcery.com> wrote:
> > On Fri, 31 May 2013 13:34:12 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> That said, I think any new patches to fpu/ need to
> >> come with an explicit statement that they can be
> >> licensed under the softfloat-2a license or GPLv2
> >> or BSD (etc etc) so they aren't an obstacle to
> >> the softfloat-2a-to-2b conversion that is in the works.
> >> [cc'd Anthony so he can correct me if I'm wrong.]
> >
> > I hereby place this one contribution (which I think wouldn't constitute a
> > copyrightable change anyway) into the Public Domain, allowing any kind of
> > usage.
> 
> I think we'd generally suggest creative commons CC0 as being
> in less of a legally grey area internationally than "public
> domain", if you're happy with that.

Using Creative Commons' CC0 is likewise fine.

> Either way,
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks.


Oh, and as I saw you wondering in the QEMU IRC channel, why I had
bothered with this change anyway, the reason is that I have some further
SoftFloat changes forthcoming, and in the course of these stumbled over
that oddity in the floatx80ToCommonNaN function, and already submitted
that one separately asnot related to my other changes.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]