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

[J. Mayer]

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

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

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