Re: [PATCH v5 10/21] target/loongarch: Add floating point arithmetic instruction translation

[Richard Henderson <richard.henderson@linaro.org>]

On 9/14/21 6:00 AM, Song Gao wrote:
> +static void update_fcsr0_mask(CPULoongArchState *env, uintptr_t pc, int mask)
> +{
> +    int flags = get_float_exception_flags(&env->fp_status) & (~mask);
> +
> +    if (!flags) {
> +        SET_FP_CAUSE(env->fcsr0, flags);
> +        return;

If mask != 0, we may unintentionally leave fp_status.flags != 0, which will then be 
incorporated into the next fp operation.

> +    }
> +
> +    flags = ieee_ex_to_loongarch(flags);
> +    set_float_exception_flags(0, &env->fp_status);

I think this set should move above the if.

> +static bool trans_fmax_s(DisasContext *ctx, arg_fmt_fdfjfk *a)
> +{
> +    tcg_gen_umax_i64(cpu_fpr[a->fd], cpu_fpr[a->fj], cpu_fpr[a->fk]);

Um, this is an integer operation.  You want float32_maxnum.
Likewise for the other min/max operations.

> +static bool trans_fabs_s(DisasContext *ctx, arg_fmt_fdfj *a)
> +{
> +    tcg_gen_ext32u_tl(cpu_fpr[a->fj], cpu_fpr[a->fj]);

You shouldn't be modifying fj.

> +    tcg_gen_abs_i64(cpu_fpr[a->fd], cpu_fpr[a->fj]);

This is an integer operation.  You want

     tcg_gen_andi_i64(cpu_fpr[a->fd], cpu_fpr[a->fj],
                      MAKE_64BIT_MASK(0, 31));

> +static bool trans_fabs_d(DisasContext *ctx, arg_fmt_fdfj *a)
> +{
> +    tcg_gen_abs_i64(cpu_fpr[a->fd], cpu_fpr[a->fj]);

Similarly.

> +/*
> + * LoongArch requires NaN-boxing of narrower width floating point values.
> + * This applies when a 32-bit value is assigned to a 64-bit FP register.
> + */

Um, this comment does not reflect the language in the v1.00 manual.

The actual language says that the upper 32 bits are undefined ("can be any value").  We 
should state that here, and then that QEMU chooses to nanbox, because it is most likely to 
show guest bugs early.


r~