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

[J. Mayer]

On Sat, 2007-11-10 at 19:09 +0100, Aurelien Jarno wrote:
> 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 

For those targets that can make the distinction between rounded sNaN and
infinite case, I don't see why there should be a problem.
But, I realize now, as you, that PowerPC is really not a good example
for comparison as it does not know about 32 bits floats. And I also
realize it cannot use the softmmu routines to round 64 bits floats to
single precision.

> >>> 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.

When using the native softfloat implementation, you don't have any flags
set then cannot raise any sNaN exception. This check allow use of the
native softfloat.
I also checked the softfloat implementation (not the native one) for
fmul and fdiv and it appears that it does not give me the needed
informations to raise the correct exceptions for the PowerPC targets.
For example, it does not seem to me that the 0 x infinity case is
properly trapped
And it does not seem to me to make a distinction between NaN operands
and invalid operations or between different "classes" of invalid
operations then do not allow a proper update of the PowerPC FPU flags.
Then, for PowerPC, checking the precise case "by hand" and using the
native softfloat implementation seems more accurate if I want to have
precise and correct results (this is sure not the case, due to all the
bugs I must have accumulated in that code !).

> 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.
[...]

> 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.

Yes, you're right, PowerPC have to use its own conversion routine. And
it seems it also needs specific routines to load and store single
precision floats to / from double precision registers.

-- 
J. Mayer <address@hidden>
Never organized