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

[Michael Tokarev]

31.05.2013 13:39, Thomas Schwinge wrote:
> Signed-off-by: Thomas Schwinge <address@hidden>
> ---
>  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