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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 31/05/2013 14:07, Michael Tokarev ha scritto:
> 31.05.2013 13:39, Thomas Schwinge wrote:
>> Signed-off-by: Thomas Schwinge <thomas@codesourcery.com>
>> ---
>>  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.

I agree.  It's also not trivial.

Paolo