Re: [Qemu-devel] [PATCH] Fix NaN handling in softfloat

[Aurelien Jarno <aurelien@aurel32.net>]

J. Mayer a écrit :
> On Sat, 2007-11-10 at 10:35 +0100, Aurelien Jarno wrote:
>> J. Mayer a écrit :
>>> On Thu, 2007-11-08 at 00:05 +0100, Aurelien Jarno wrote:
>>>> On Tue, Nov 06, 2007 at 09:01:13PM +0100, J. Mayer wrote:
>>>>> On Sat, 2007-11-03 at 22:28 +0100, Aurelien Jarno wrote: 
>>>>>> On Sat, Nov 03, 2007 at 02:06:04PM -0400, Daniel Jacobowitz wrote:
>>>>>>> On Sat, Nov 03, 2007 at 06:35:48PM +0100, Aurelien Jarno wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> The current softfloat implementation changes qNaN into sNaN when 
>>>>>>>> converting between formats, for no reason. The attached patch fixes
>>>>>>>> that. It also fixes an off-by-one in the extended double precision
>>>>>>>> format (aka floatx80), the mantissa is 64-bit long and not 63-bit
>>>>>>>> long.
>>>>>>>>
>>>>>>>> With this patch applied all the glibc 2.7 floating point tests
>>>>>>>> are successfull on MIPS and MIPSEL.
> [...]
>>>> Anyway there is no way to do that in the target specific code *after 
>>>> the conversion*, as the detection of a mantissa being nul when 
>>>> converting from double to single precision can only be done when both
>>>> values are still known. In other words when the value is not fixed 
>>>> during the conversion, the value 0x7f800000 can either be infinity or a
>>>> conversion of NaN from double to single precision, and thus is it not
>>>> possible to fix the value afterwards in the target specific code.
>>> I don't say you have to return an infinity when the argument is a qNaN.
>>> I just say you have to return a qNaN in a generic way.  Just return sign
>>> | 0x7f800000 | mantissa, which is the more generic form and seems to me
>>> to even be OK for sNaNs. It's even needed for some target (not to say
>> 0x7f800000 is actually not a NaN, but infinity.
>>
>>> PowerPC) that specify that the result have to be equal to the operand
>>> (in the single precision format, of course) in such a case. This is
>>> simpler, it ensures that any target could then detect the presence of a
>>> NaN, know which one, and can then adjust the value according to its
>>> specification if needed.
>>> I then still can'tl see any reason of having target specific code in
>>> that area.
>> Ok, let's give an example then. On MIPS let's say you want to convert
>> 0x7ff0000000000001 (qNaN) to single precision. The mantissa shifted to
>> the right become 0, so you have to generate a new value. As you
>> proposed, let's generate a "generic value" 0x7fc00000 in the softfloat
>> routines. This value has to be converted to 0x7fbfffff in the MIPS
>> target code.
> 
> OK, the values that can cause a problem is all values that would have a
> zero mantissa once rounded to sinlge-precision. As the PowerPC requires
> that the result would have a zero mantissa (and the result class set to

Are you sure of that? According to IEEE 754 a zero mantissa is not a
NaN. And tests on a real machine shows different results.
0x7ff0000000000001 is converted to 0x7fc00000 on a 740/750 CPU.


> qNan), I can see no way to handle this case in the generic code. And
> even adding a "#ifdef TARGET_PPC" won't solve the problem as the PowerPC
> code would not be able to make the distinction between infinity case and
> qNaN case. Then, the only solution, as you already mentioned, is to
> check for qNaN before calling the rounding function. As the target
> emulation code already has to check for sNaN to be able to raise an
> exception when it's needed, checking for qNaN would cost nothing more;

Except this is currently done *after* the call to the rounding function,
using the flags returned by the softmmu routines. Doing a check before
and after would slow down the emulation.

> just have to change the check if (float64_is_signaling_nan) check with a
> check for NaN and handle the two cases by hand. I can see no other way
> to have all cases handled for all targets specific cases, do you ?
> 

If you can confirm that the all PowerPC CPU are IEEE compliant and
should behave like the 740/750, the patch I proposed is another way to
be correct on all target, including PowerPC. But it seems you don't
really like to add target specific code in the softmmu routines.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net