Re: [PATCH v11 14/26] target/loongarch: Add floating point load/store instruction translation

[Richard Henderson]

On 11/19/21 7:13 AM, Song Gao wrote:
> +static bool gen_fload_imm(DisasContext *ctx, arg_fr_i *a,
> +                          MemOp mop, bool nanbox)

Don't pass nanbox, as it can be determined from mop.

I think you should split out

static void maybe_nanbox_load(TCGv freg, MemOp mop)
{
    if ((mop & MO_SIZE) == MO_32) {
        gen_nanbox_s(freg, freg);
    }
}

for use in the 4 different fload functions.

> +static bool gen_fstore_imm(DisasContext *ctx, arg_fr_i *a,
> +                           MemOp mop, bool nanbox)

Don't pass nanbox, because it's useless for stores.

> +    if (nanbox) {
> +        gen_nanbox_s(cpu_fpr[a->fd], cpu_fpr[a->fd]);
> +    }

(1) nanboxing not needed for store,
(2) incorrect to modify fd.

> +static bool gen_fload_tl(DisasContext *ctx, arg_frr *a,
> +                         MemOp mop, bool nanbox)

Similarly.

Since the integer version is called gen_loadx, should this one be called 
gen_floadx?

> +static bool gen_fstore_tl(DisasContext *ctx, arg_frr *a,
> +                          MemOp mop, bool nanbox)
...
> +static bool gen_fload_gt(DisasContext *ctx, arg_frr *a,
> +                         MemOp mop, bool nanbox)
...
> +static bool gen_fstore_gt(DisasContext *ctx, arg_frr *a,
> +                          MemOp mop, bool nanbox)
...
> +static bool gen_fload_le(DisasContext *ctx, arg_frr *a,
> +                         MemOp mop, bool nanbox)
...
> +static bool gen_fstore_le(DisasContext *ctx, arg_frr *a,
> +                          MemOp mop, bool nanbox)

Simiarly.

> +TRANS(fld_s, gen_fload_imm, MO_TESL, true)

Use TEUL for everything here, because you don't need sign extension.


r~