Patchwork fpu: Simplify floatx80ToCommonNaN function.

login
register
mail settings
Submitter Thomas Schwinge
Date May 31, 2013, 9:39 a.m.
Message ID <1369993154-17690-1-git-send-email-thomas@codesourcery.com>
Download mbox | patch
Permalink /patch/247981/
State New
Headers show

Comments

Thomas Schwinge - May 31, 2013, 9:39 a.m.
Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
---
 fpu/softfloat-specialize.h |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
Thomas Schwinge - May 31, 2013, 1:01 p.m.
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
Thomas Schwinge - June 3, 2013, 11:05 a.m.
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

Patch

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;
 }