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

[Aurelien Jarno <aurelien@aurel32.net>]

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.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  fpu/softfloat-specialize.h |  160 +++++++++++++++++++++++++++----------------
>  1 files changed, 100 insertions(+), 60 deletions(-)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index 8e6aceb..3015480 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -134,6 +134,52 @@ static float32 commonNaNToFloat32( commonNaNT a )
>  }
>  
>  /*----------------------------------------------------------------------------
> +| Select which NaN to propagate for a two-input operation.
> +| IEEE754 doesn't specify all the details of this, so the
> +| algorithm is target-specific.
> +| The routine is passed various bits of information about the
> +| two NaNs and should return 0 to select NaN a and 1 for NaN b.
> +| Note that signalling NaNs are always squashed to quiet NaNs
> +| by the caller, by flipping the SNaN bit before returning them.
> +|
> +| aIsLargerSignificand is only valid if both a and b are NaNs
> +| of some kind, and is true if a has the larger significand,
> +| or if both a and b have the same significand but a is
> +| positive but b is negative. It is only needed for the x87
> +| tie-break rule.
> +*----------------------------------------------------------------------------*/
> +
> +static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
> +                    flag aIsLargerSignificand)
> +{
> +    /* This implements x87 NaN propagation rules:
> +     * SNaN + QNaN => return the QNaN
> +     * two SNaNs => return the one with the larger significand, silenced
> +     * two QNaNs => return the one with the larger significand
> +     * SNaN and a non-NaN => return the SNaN, silenced
> +     * QNaN and a non-NaN => return the QNaN
> +     *
> +     * If we get down to comparing significands and they are the same,
> +     * return the NaN with the positive sign bit (if any).
> +     */
> +    if (aIsSNaN) {
> +        if (bIsSNaN) {
> +            return aIsLargerSignificand ? 0 : 1;
> +        }
> +        return bIsQNaN ? 1 : 0;
> +    }
> +    else if (aIsQNaN) {
> +        if (bIsSNaN || !bIsQNaN)
> +            return 0;
> +        else {
> +            return aIsLargerSignificand ? 0 : 1;
> +        }
> +    } else {
> +        return 1;
> +    }
> +}

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.

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.

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;

However if we finally implement correct NaN propagation in the 
softfloat routines, it's better to have everything implemented there.

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