[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH] fpu: Simplify floatx80ToCommonNa
From: |
Thomas Schwinge |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH] fpu: Simplify floatx80ToCommonNaN function. |
Date: |
Fri, 31 May 2013 15:01:10 +0200 |
User-agent: |
Notmuch/0.9-101-g81dad07 (http://notmuchmail.org) Emacs/23.4.1 (x86_64-pc-linux-gnu) |
Hi!
On Fri, 31 May 2013 13:34:12 +0100, Peter Maydell <address@hidden> wrote:
> On 31 May 2013 13:07, Michael Tokarev <address@hidden> 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
pgpHwUWgCYY9M.pgp
Description: PGP signature