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

[Thomas Schwinge <thomas@codesourcery.com>]

[-- 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 --]