Re: [Qemu-devel] [PATCH 1/2] softfloat: abstract out target-specific NaN propagation rules

[Aurelien Jarno]

On Sun, Jan 02, 2011 at 02:04:11PM +0000, Peter Maydell wrote:
> On 2 January 2011 13:23, Aurelien Jarno <address@hidden> wrote:
> > On Thu, Dec 16, 2010 at 11:51:17AM +0000, Peter Maydell wrote:
> >> IEEE754 doesn't specify precisely what NaN should be returned as
> >> the result of an operation on two input NaNs. This is therefore
> >> target-specific. Abstract out the code in propagateFloat*NaN()
> >> which was implementing the x87 propagation rules, so that it
> >> can be easily replaced on a per-target basis.
> 
> > I am basically find with the idea. I have tried to implement that for
> > MIPS, but it seems your current implementation doesn't allow correct
> > propagation for MIPS: if one of the two operand are a sNaN, the result
> > should be a *default* qNaN.
> 
> So if the input is a QNaN it's passed through but if it's an SNaN
> you get the default QNaN? I guess it makes sense since MIPS

Correct.

> has SNAN_BIT_IS_ONE and you can't just silence a NaN by
> flipping the bit (because you might end up with a non-NaN if
> the final significand is all-zeroes). [...and the existing code that
> tries to do that is therefore wrong, presumably for both MIPS
> and HPPA.]

This is also correct. We should probably change it to the default sNaN
if the resulting significand is all-zeroes.

> Could we have a target-specific "silence this SNaN" function?

You mean a target-specific version of floatXX_maybe_silence_nan()?
At least having a default version (for !SNAN_BIT_IS_ONE) and a version
for MIPS, HPPA (which is btw not emulated), etc.

> Then the top level functions could use those rather than doing
> their own bit-flipping, and I think that would do the right thing
> for MIPS (you'd implement silence-NaN as "return the default
> QNaN", and you implement pickNaN() to return the SNaN.)

It seems, it would be the good way to do it. I am going to do a quick
hack to see if this solution can work and if it is the case (it seems
to be), apply your patch.

> > It seems that we should pass the operands to the pickNaN() function and
> > return the result instead of a flag. That means having one pickNaN
> > function per float type, but that can probably be handled by macros or
> > by having a common function for targets on which its possible to do so.
> 
> As you might have guessed I was definitely trying to avoid having
> to actually pass the operands to pickNaN()...

I agree with you that if it can be avoided, it's the best option.

> > Note however that the current implementation provides the correct
> > result, as the result is converted in op_helper.c:
> >
> >    if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID)
> >            wt2 = FLOAT_QNAN32;
> 
> ...incidentally, I don't know MIPS but this code looks a bit suspect to me:
>     set_float_exception_flags(0, &env->active_fpu.fp_status);            \
>     wt2 = float32_ ## name (fst0, fst1, &env->active_fpu.fp_status);     \
>     update_fcr31();                                                \
>     if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID)                \
>         wt2 = FLOAT_QNAN32;                                        \
> 
> Isn't it clearing the FP exception flags for each instruction when
> they ought to be cumulative?
> 

It only clears the softfloat ones, the real ones are kept in CPU
register fcr31, which is updated by update_fcr31(). This allows to
correctly emulate fcr31, as it contains a part that cumulates IEEE754 
flags and can be reset by software, and a part that cumulates exception
flags, which triggers exceptions if enabled, and which can be reset
independently.

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