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

[Aurelien Jarno]

J. Mayer a écrit :
> On Sat, 2007-11-10 at 17:15 +0100, Aurelien Jarno wrote:
>> 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.
> 
> First, please note that a PowerPC do not have any single precision
> register nor internal representation. The operation here is "round to
> single precision" (frsp) but the result is still a 64 bits float. Then
> the result is more likely to be 0x7fc0000000000000.
> 0x7FF0000000000001 seems to be a SNaN, according to what I see in the
> PowerPC specification. Then the result is OK: when no exception is
> raised, SNaN is converted to QNaN during rounding to single operation
> (please see below).
> What about 0x7FF8000000000001, which is a QNaN ? According to the
> PowerPC specification, this should be rounded to 0x7FF8000000000000
> which is also a QNaN, then is also OK. Then rounding the mantissa and
> copying sign || exponent || mantissa would, in fact, always be OK in the
> PowerPC case.
> What seem to appear to me now is that the problems are due to the fact
> Mips have an inverted representation of SNaN / QNaN (if I understood
> well) that do not allow distinction between a rounded QNaN and an
> infinity...

Nope it is not due to the fact that MIPS uses an "inverted"
representation. It is the same problem on x86 or other target, except
that they can allow the distinction between a rounded SNaN and an
infinity. The problem is present on all targets that can represent a
single precision FP value.


>>> 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.
> 
> On PowerPC at least, you have to check operands for sNaN _before_ doing
> any floating-point operation and check the result _after_ having done
> the floating-point computation in order to set the flags.
>
> The sNaN operand exception must be raised before any computation is
> done, because it's related to the operation operands which may be lost
> during the operation if the target register is the same than an operand
> one.

I don't really understand why this is necessary to do it before *and*
after. The softmmu routines already does that job, and compute the
correct flags according to the operand. The only remaining operation for
the target specific code is to copy the softmmu flags to the target FP
status register and trigger exception if necessary.

Anyway that's not the point here, what to remember is that most target
only check operand after the softmmu routine

> The other exceptions, based on the result of the operation, must be
> raised after the result has been computed and stored.
> Then, I can see way not to check for SNaN first. And I guess this is the
> case for most FPU...
> 
>>> 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.
> 
> The code you proposed is not correct for PowerPC.
> Here's what the PowerPC specification says:
> Just a few precisions before:
> * bit 0 is MSB in IBM representation
> * notation (XXX)m:n means bits from m to n (included) of XXX
> * FRB is the operand register
> * FRT is the result register
> * FPSCR is the FPU flags register
> 
> Floating point round to single-precision model:
> (just keeping the NaN parts and adding comments, hope they are
> releveant !)
> [...]
> if ((FRB) 1:11 == 2047) then
>     if ((FRB)12:63 == 0) then goto Infinity Operand /* zero mantissa */
>     if ((FRB)12 == 1) then goto QNaN Operand           /* mantissa MSB
> is one */
>    if ((FRB)12 == 0) and ((FRB)13:63 > 0) then goto SNaN Operand /*
> mantissa MSB is zero and mantissa != 0 */ 
> 
> [...]
> 
> QNaN Operand:
>   FRT <- (FRB)0:34 || 0 /* Copy the highest 35 bits of the operand to
> result */
>   (FPSCR)FPRF <- QNaN  /* Set result class to QNaN */
>   (FPSCR)FRFI <- 0b00   /* Reset result sign / zero flags */
> 
> SNaN Operand:
> 1 <- (FPSCR)VXSNAN <- 1 /* Set SNaN exception flag */
> if ((FPSCR)VE == 0) then /* SNaN exceptions are disabled */
>     (FRT)0:11 <- (FRB)0:11 /* Copy sign / exponent from the operand to
> result */
>     (FRT)12 <- 1 /* Ensure the the highest bit of mantissa is 1 */
>     (FRT)13:63 <- (FRB)13:34 || 0 /* Copy the bits 13:34 from the
> operand to result */
>     (FPSCR)FPRF <- QNaN /* set the result class to QNaN */
> (FPSCR)FRFI <- 0b00 /* Reset result sign / zero flags */
> 
> Then, as mentionned previously, there is no problem in this model,
> except the fact that it keeps more bits than the ones that would fit in
> a single-precision float in case of NaN: it keeps 35 bits of the
> operand, then this cannot be handled by the generic routine that would
> just keep 32 bits... Then I will still have to check this case at the
> same place I check for sNaN in order to always have the correct result.
> If the SNaN / QNaN representation is inverted, compared to this model,
> then yes, you have a problem with this method. But I still don't think
> adding target specific code in generic code that should not be target
> dependant is the solution. Having a define signaling that QNaN is 1/0 //
> SNaN is 0/1 would be more generic and checking that flag (and not
> TARGET_XXX) in that code would be more acceptable. But I keep thinking
> that a check on the operand is needed before computation for most CPUs,
> then this hack should not be needed...
> 

It looks like the problem is different on PowerPC, because it does not
know about single precision FP. Then I wonder why to use "double
precision to single precision conversion" softmmu routine on PowerPC.
This routine is working correctly (with the patch I submitted) for all
target, but PowerPC.

>From my point of view the best is to create a "round to single
precision" softmmu subroutine that takes a double precision for both
input and output for PowerPC.

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