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

[Peter Maydell]

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.

> 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