Re: [Qemu-devel] [PATCH 3/3] target-ppc: fix sNaN propagation

[Peter Maydell <peter.maydell@linaro.org>]

On 2 January 2011 08:39, Aurelien Jarno <aurelien@aurel32.net> wrote:
> The current FPU code returns 0.0 if one of the operand is a
> signaling NaN and the VXSNAN exception is disabled.
>
> fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> exception as the operand should be propagated instead of a new
> qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> only for the exception generation (if enabled), and use the softfloat
> code to correctly compute the result.
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

> @@ -1410,10 +1418,10 @@ uint64_t helper_frsp (uint64_t arg)
>     if (unlikely(float64_is_signaling_nan(farg.d))) {
>         /* sNaN square root */
>        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> -    } else {
> -       f32 = float64_to_float32(farg.d, &env->fp_status);
> -       farg.d = float32_to_float64(f32, &env->fp_status);
>     }
> +    f32 = float64_to_float32(farg.d, &env->fp_status);
> +    farg.d = float32_to_float64(f32, &env->fp_status);
> +
>     return farg.ll;
>  }

Most of these changes are to ignoring the result from
fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN),
but this one leaves it assigning a value to farg.ll -- is there
any reason for that? (It looks like the assignment gets
immediately overwritten by the assignment to farg.d later.)

> @@ -1460,11 +1468,11 @@ uint64_t helper_fres (uint64_t arg)
>     if (unlikely(float64_is_signaling_nan(farg.d))) {
>         /* sNaN reciprocal */
>         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> -    } else {
> -        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> -        f32 = float64_to_float32(farg.d, &env->fp_status);
> -        farg.d = float32_to_float64(f32, &env->fp_status);
>     }
> +    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> +    f32 = float64_to_float32(farg.d, &env->fp_status);
> +    farg.d = float32_to_float64(f32, &env->fp_status);
> +
>     return farg.ll;
>  }
>

Ditto for this hunk.

Looks plausible to me other than that.

-- PMM