[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/6] softfloat: use float{32, 64, x80, 128}_mayb
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH 4/6] softfloat: use float{32, 64, x80, 128}_maybe_silence_nan() |
Date: |
Mon, 3 Jan 2011 23:44:54 +0100 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
On Mon, Jan 03, 2011 at 05:34:17PM +0000, Peter Maydell wrote:
> On 3 January 2011 14:34, Aurelien Jarno <address@hidden> wrote:
> > Use float{32,64,x80,128}_maybe_silence_nan() instead of toggling the
> > sNaN bit manually. This allow per target implementation of sNaN to qNaN
> > conversion.
>
> > @@ -237,15 +237,11 @@ static float32 propagateFloat32NaN( float32 a,
> > float32 b STATUS_PARAM)
> > aIsSignalingNaN = float32_is_signaling_nan( a );
> > bIsNaN = float32_is_quiet_nan( b );
> > bIsSignalingNaN = float32_is_signaling_nan( b );
> > + a = float32_maybe_silence_nan(a);
> > + b = float32_maybe_silence_nan(b);
> > av = float32_val(a);
> > bv = float32_val(b);
> > -#if SNAN_BIT_IS_ONE
> > - av &= ~0x00400000;
> > - bv &= ~0x00400000;
> > -#else
> > - av |= 0x00400000;
> > - bv |= 0x00400000;
> > -#endif
> > +
> > if ( aIsSignalingNaN | bIsSignalingNaN ) float_raise(
> > float_flag_invalid STATUS_VAR);
> >
> > if ((bits32)(av<<1) < (bits32)(bv<<1)) {
>
> The place you've put the maybe_silence_nan() calls means that
> the calculation of aIsLargerSignificand is done based on the silenced
> values rather than on the input values, which I think is wrong (it
> is inconsistent with the *isSignalingNaN and *isNaN flags).
>
> It doesn't make any difference in practice, because the only thing
> that uses it is the x87 rules, and they only look at aIsLargerSignificand
> if both a and b are the same type of NaN, in which case by x87
> silencing rules the comparison gives the same result whether it's
> before or after silencing. But I think conceptually it would be better
> to call float*_maybe_silence_nan() after the calculation of
> aIsLargerSignificand.
I agree it should not make any change, but that it is conceptually
wrong. I'll fix that in v2.
> Unrelated trivial cleanup which I've just noticed and which you might
> want to throw into v2 of this patchset:
> > bIsNaN = float32_is_quiet_nan( b );
>
> ...these variables would be better named as aIsQNan, bIsQNaN in
> these propagate* functions.
>
Probably to "aIsQuietNaN" to match the "aIsSignalingNaN" name.
I'll send the v2 tomorrow.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
address@hidden http://www.aurel32.net
[Qemu-devel] [PATCH 2/6] softfloat: fix float{32, 64}_maybe_silence_nan() for MIPS, Aurelien Jarno, 2011/01/03
[Qemu-devel] [PATCH 4/6] softfloat: use float{32, 64, x80, 128}_maybe_silence_nan(), Aurelien Jarno, 2011/01/03
[Qemu-devel] [PATCH 3/6] softfloat: add float{x80, 128}_maybe_silence_nan(), Aurelien Jarno, 2011/01/03
[Qemu-devel] [PATCH 5/6] target-mips: Implement correct NaN propagation rules, Aurelien Jarno, 2011/01/03
[Qemu-devel] [PATCH 6/6] target-ppc: Implement correct NaN propagation rules, Aurelien Jarno, 2011/01/03