Re: [PATCH v2 13/22] target/loongarch: Add floating point arithmetic instruction translation

[Richard Henderson]

On 7/26/21 9:17 PM, Song Gao wrote:
> > I think this should be as simple as
> >
> >    gen_helper_fp_add_s(cpu_fpu[a->fd], cpu_env,
> >                        cpu_fpu[a->fj], cpu_fpu[a->fk]);
> >
> > I also think that loongarch should learn from risc-v and change the architecture to 
> > "nan-box" single-precision results -- fill the high 32-bits with 1s.  This is 
> > an SNaN representation for double-precision and will immediately fail when incorrectly 
> > using a single-precision value as a double-precision input.
> >
> > Thankfully the current architecture is backward compatible with nan-boxing.
>
> by this method,  the trans_fadd_s is
>
> static bool trans_fadd_s(DisasContext *ctx, arg_fadd_s * a)
> {
>      TCGv_i64 fp0, fp1;
>
>      fp0 = tcg_temp_new_i64();
>      fp1 = tcg_temp_new_i64();
>
>      check_fpu_enabled(ctx);
>      gen_load_fpr64(fp0, a->fj);
>      gen_load_fpr64(fp1, a->fk);
>      gen_helper_fp_add_s(fp0, cpu_env, fp0, fp1);
>
>      gen_check_nanbox_s(fp0, fp0); /* from riscv */
>
>      gen_store_fpr64(fp0, a->fd);
>
>      tcg_temp_free_i64(fp0);
>      tcg_temp_free_i64(fp1);
>
>      return true;
> }

A few points here:

(1) You do not need gen_load_fpr64 and gen_store_fpr64 at all.
    These were from mips to deal with the varying fpu sizes.

(2) If we need to call a helper, then the helper as much of
    the work a possible.  Therefore the nanboxing should be
    done there.  See riscv/fpu_helper.c, and the use of
    nanbox_s within that file.

(3) Again, use a helper function:

static bool gen_binary_fp(DisasContext *ctx, arg_fmt_fdfjfk *a,
                          void (*func)(TCGv_i64, TCGv_env,
                                       TCGv_i64, TCGv_i64))
{
    if (check_fpu_enabled(ctx)) {
        func(cpu_fpr[a->fd], cpu_env,
             cpu_fpr[a->fj], cpu_fpr[a->fk]);
    }
    return true;
}

TRANS(fadd_s, gen_binary_fp, gen_helper_fp_add_s)
TRANS(fadd_d, gen_binary_fp, gen_helper_fp_add_d)

> uint64_t helper_fp_add_s(CPULoongArchState *env, uint64_t fp, uint64_t fp1)
> {
>      uint32_t fp2;
>
>      fp2 = float32_add((uint32_t)fp, (uint32_t)fp1, &env->active_fpu.fp_status);
>      update_fcsr0(env, GETPC());
>      return (uint64_t)fp2;
> }

with return nanbox_s(fp2);


r~