Re: [Qemu-devel] [PATCH] * include/fpu/softfloat.h (floatx80_invalid_encoding): Handle m68k specific infinity pattern.

[Laurent Vivier <laurent@vivier.eu>]

Le 18/09/2019 à 11:59, Alex Bennée a écrit :
> 
> Pierre Muller <pierre@freepascal.org> writes:
> 
>>   Hi Thomas,
>>
>>   I tried to use git format-patch -s below,
>> and change the commit message that appears below:
>>
>>
>> muller@gcc123:~/gnu/qemu/qemu$ git format-patch -s a017dc6d43aaa4ffc7be40ae3adee4086be9cec2^
>> 0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch
>> muller@gcc123:~/gnu/qemu/qemu$ cat
>> 0001-Fix-floatx80_invalid_encoding-function-for-m68k-cpu.patch
> 
> It's best to send the patches directly (i.e. don't include them in a
> larger email). This is because when maintainers apply the email they end
> up with a bunch of additional stuff and the corrupting of subject line.
> 
>> From a017dc6d43aaa4ffc7be40ae3adee4086be9cec2 Mon Sep 17 00:00:00 2001
>> From: Pierre Muller <pierre@freepascal.org>
>> Date: Wed, 18 Sep 2019 08:04:19 +0000
>> Subject: [PATCH]    Fix floatx80_invalid_encoding function for m68k cpu
>>
>>     As m68k accepts different patterns for infinity,
>>     and additional test for valid infinity must be added
>>     for m68k cpu target.
>>
>> Signed-off-by: Pierre Muller <pierre@freepascal.org>
>> ---
>>  include/fpu/softfloat.h | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
>> index ecb8ba0114..dea24384e9 100644
>> --- a/include/fpu/softfloat.h
>> +++ b/include/fpu/softfloat.h
>> @@ -685,10 +685,17 @@ static inline int floatx80_is_any_nan(floatx80 a)
>>  | pseudo-infinities and un-normal numbers. It does not include
>>  | pseudo-denormals, which must still be correctly handled as inputs even
>>  | if they are never generated as outputs.
>> +| As m68k accepts different patterns for infinity, thus an additional test
>> +| for valid infinity value must be added for m68k CPU.
>>  *----------------------------------------------------------------------------*/
>>  static inline bool floatx80_invalid_encoding(floatx80 a)
>>  {
>> +#if defined (TARGET_M68K)
>> +    return ((a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0)
>> +           && (! floatx80_is_infinity(a));
>> +#else
>>      return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
>> +#endif
>>  }
> 
> As most of the test is the same we could rewrite this to:
> 
>  bool invalid = (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
>  #if defined (TARGET_M68K)
>  invalid &= !floatx80_is_infinity(a)
>  #endif
>  return invalid;
> 
> The compiler should be able to simplify the logic from there.
> 
> Do we have any test cases that we could add to tests/tcg/m68k?
> 
>>
>>  #define floatx80_zero make_floatx80(0x0000, 0x0000000000000000LL)
> 

I think patch from Pierre doesn't treat all the problem and don't rely 
on any specification.

I proposed a patch years ago that is closer to the hardware behaviour:

/*----------------------------------------------------------------------------
| Return whether the given value is an invalid floatx80 encoding.
| Invalid floatx80 encodings arise when the integer bit is not set, but
| the exponent is not zero. The only times the integer bit is permitted to
| be zero is in subnormal numbers and the value zero.
| This includes what the Intel software developer's manual calls pseudo-NaNs,
| pseudo-infinities and un-normal numbers. It does not include
| pseudo-denormals, which must still be correctly handled as inputs even
| if they are never generated as outputs.
*----------------------------------------------------------------------------*/
static inline bool floatx80_invalid_encoding(floatx80 a)
{
#if defined(TARGET_M68K)
    /*-------------------------------------------------------------------------
    |  M68000 FAMILY PROGRAMMER’S REFERENCE MANUAL
    |  1.6.2 Denormalized Numbers
    |  Since the extended-precision data format has an explicit integer bit,
    |  a number can be formatted with a nonzero exponent, less than the maximum
    |  value, and a zero integer bit.  The IEEE 754 standard does not define a
    |  zero integer bit. Such a number is an unnormalized number. Hardware does
    |  not directly support denormalized and unnormalized numbers, but
    |  implicitly supports them by trapping them as unimplemented data types,
    |  allowing efficient conversion in software.
    *------------------------------------------------------------------------*/
    return 0;
#else
    return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
#endif
}

But in fact I think this kind of number should raise an exception.

I tried to do this in:

https://github.com/vivier/qemu-m68k/commit/5bd7742437a3c770373734add5ab452e8df4e621

but it needs more work and for the moment doesn't fix the problem Pierre is trying to fix.

Thanks,
Laurent