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

[Peter Maydell]

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

Could we have a target-specific "silence this SNaN" function?
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 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()...

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

-- PMM