Re: [Qemu-devel] [PATCH 1/6] softfloat: remove HPPA specific code

[Aurelien Jarno]

On Thu, Jan 06, 2011 at 03:34:38PM +0000, Peter Maydell wrote:
> On 6 January 2011 14:35, Aurelien Jarno <address@hidden> wrote:
> > On Wed, Jan 05, 2011 at 11:13:06PM +0000, Stuart Brady wrote:
> >>    Is there any plan to deal with use of float*_is_quiet_nan(), where
> >>    float*_is_any_nan() was intended?  These should really either be
> >>    fixed (and tested), or if not, a FIXME should be added.
> >
> > The problem with these functions is that they are used in target-*/
> > and not directly part of the softfloat code.
> >
> > I have reviewed MIPS and PowerPC targets, they both use these functions
> > correctly.
> 
> MIPS looks OK. However PPC has this in helper_compute_fprf():
> 
>     if (unlikely(float64_is_quiet_nan(farg.d))) {
>         if (float64_is_signaling_nan(farg.d)) {
>             /* Signaling NaN: flags are undefined */
>             ret = 0x00;
>         } else {
>             /* Quiet NaN */
>             ret = 0x11;
>         }
> 
> which is definitely wrong because the first case can't be reached.
> The outer if should be is_any_nan(), I think.

Correct.

> In helper_fnmadd() and helper_fnmsub():
>         if (likely(!float64_is_quiet_nan(farg1.d)))
>             farg1.d = float64_chs(farg1.d);
> 
> is I think OK but somebody else might like to check.

After reading the manual again, it seems float64_is_nan() should be used
here. The idea is that fnmadd returns the negated value of fmadd, and
that NaN should be propagated as fnmadd was a single instruction. In
QEMU chs changes the sign bit even if the value is NaN. Quoting the
manual:

| This instruction produces the same result as would be obtained by 
| using the Floating Multiply-Add instruction and then negating the 
| result, with the following exceptions. 
| * QNaNs propagate with no effect on their "sign" bit.
| * QNaNs that are generated as the result of a disabled Invalid 
|   Operation Exception have a "sign" bit of 0.
| * SNaNs that are converted to QNaNs as the result of a disabled 
| Invalid Operation Exception retain the "sign”"bit of the SNaN.

> Similarly I'm dubious about uses in helper_fsel, helper_fcmpu
> and helper_fcmpo, 

I confirm the issue for this three helpers, tested on real hardware.

> efsctsi, efsctui, efsctsiz, efsctuiz, efsctsf,
> efsctuf and all the helper_efd* functions. I haven't actually
> checked them all, but for instance efdctsi in the Power ISA
> 2.03 spec says "NaNs are converted as though they were zero",
> but qemu's code says:
>     /* NaN are not treated the same way IEEE 754 does */
>     if (unlikely(float64_is_quiet_nan(u.d)))
>         return 0;
> 
> which is not going to do the right thing for signaling NaNs.
> 

Also correct.

Looks like I have read the code a bit quickly... Thanks for looking at
it, I will send a patch later.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net