Re: [Qemu-devel] [PATCH v2 9/9] target-i386: add support for FPU exceptions

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

On 23 May 2011 22:42, Aurelien Jarno <aurelien@aurel32.net> wrote:
> This patch adds support for FPU exceptions. It keeps the exception in
> the softfloat status, and copy them back to env->fpus when needed by
> oring them. When loading a new value to env->fpus, it starts with a
> clean softfloat status.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

gdbstub.c still looks at env->fpus directly without calling
cpu_x86_update_fpus() first; it should probably go through
helper_fnstsw() now (or if you like a utility function that does what
helper_fnstsw() does now).

Similarly the code in helper_fstenv() and cpu_pre_save() that does:
    cpu_x86_update_fpus(env);
    SOMETHING = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;

could be usefully abstracted away into a call to this utility fn.

For the rest, it looks OK, but the cpu_x86_update_fpus() and
cpu_x86_set_fpus() functions feel a bit like they're only half
an abstraction layer -- so sometimes it's OK to tweak env->fpus
directly and sometimes you need to use the functions. That means
it's tricky to tell from eyeballing or grepping the code whether an
update_fpus() call got missed out. Maybe it would be better to have
a set of functions such that you could mandate that all env->fpus
accesses went via the functions?

A random nitpick:

> @@ -4208,7 +4200,13 @@ void helper_fscale(void)
>     if (floatx80_is_any_nan(ST1)) {
>         ST0 = ST1;
>     } else {
> -        int n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
> +        int n, x;
> +
> +        /* The float to int conversion should not generate any exception. */
> +        x = get_float_exception_flags(&env->fp_status);
> +        n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
> +        set_float_exception_flags(x, &env->fp_status);
> +
>         ST0 = floatx80_scalbn(ST0, n, &env->fp_status);
>     }
>  }

...doesn't this mean you won't set the #D flag if ST1 is denormal?

I'm unconvinced about scalbn as a softfloat primitive. It gets used:
 * by ARM for the VCVT fixed-point conversions, as a combination of
   scalbn + float-to-int or int-to-float + scalbn
 * by PPC for the same reason (vctuxs, vctsxs, vcfux, vcfsx)
 * by x86 here as a float-to-int + scalbn, for FSCALE

and I think that in all three cases the attempt to implement a floating
point op by composing two softfloat primitives causes problems with
getting the floating point exception flags right. (In the fixedpoint
conversion case the issue is that for instance if the input is a
negative number we should only set InvalidOp but the scalbn is likely
to result in our also setting Inexact. But if you don't let the
scalbn set any flags at all then you have to special case when the
input is a NaN or denormal, which is equally awkward.)

-- PMM