Re: [PATCH v2 07/22] target/loongarch: Add fixed point arithmetic instruction translation

[Richard Henderson]

On 7/20/21 11:53 PM, Song Gao wrote:
> +/* Fixed point arithmetic operation instruction translation */
> +static bool trans_add_w(DisasContext *ctx, arg_add_w *a)
> +{
> +    TCGv Rd = cpu_gpr[a->rd];
> +    TCGv Rj = cpu_gpr[a->rj];
> +    TCGv Rk = cpu_gpr[a->rk];
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    if (a->rj != 0 && a->rk != 0) {
> +        tcg_gen_add_tl(Rd, Rj, Rk);
> +        tcg_gen_ext32s_tl(Rd, Rd);
> +    } else if (a->rj == 0 && a->rk != 0) {
> +        tcg_gen_mov_tl(Rd, Rk);
> +    } else if (a->rj != 0 && a->rk == 0) {
> +        tcg_gen_mov_tl(Rd, Rj);
> +    } else {
> +        tcg_gen_movi_tl(Rd, 0);
> +    }
> +
> +    return true;
> +}

Do not do all of this "if reg(n) zero" testing.

Use a common function to perform the gpr lookup, and a small callback function for the 
operation.  Often, the callback function already exists within include/tcg/tcg-op.h.

Please see my riscv cleanup patch set I referenced vs patch 6.

> +static bool trans_orn(DisasContext *ctx, arg_orn *a)
> +{
> +    TCGv Rd = cpu_gpr[a->rd];
> +    TCGv Rj = cpu_gpr[a->rj];
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    TCGv t0 = tcg_temp_new();
> +    gen_load_gpr(t0, a->rk);
> +
> +    tcg_gen_not_tl(t0, t0);
> +    tcg_gen_or_tl(Rd, Rj, t0);

tcg_gen_orc_tl.

> +static bool trans_andn(DisasContext *ctx, arg_andn *a)
> +{
> +    TCGv Rd = cpu_gpr[a->rd];
> +    TCGv Rj = cpu_gpr[a->rj];
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    TCGv t0 = tcg_temp_new();
> +    gen_load_gpr(t0, a->rk);
> +
> +    tcg_gen_not_tl(t0, t0);
> +    tcg_gen_and_tl(Rd, Rj, t0);

tcg_gen_andc_tl.

> +static bool trans_mul_d(DisasContext *ctx, arg_mul_d *a)
> +{
> +    TCGv t0, t1;
> +    TCGv Rd = cpu_gpr[a->rd];
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    t0 = get_gpr(a->rj);
> +    t1 = get_gpr(a->rk);
> +
> +    check_loongarch_64(ctx);

Architecture checks go first, before you've decided the operation is a nop.

> +static bool trans_mulh_d(DisasContext *ctx, arg_mulh_d *a)
> +{
> +    TCGv t0, t1, t2;
> +    TCGv Rd = cpu_gpr[a->rd];
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    t0 = get_gpr(a->rj);
> +    t1 = get_gpr(a->rk);
> +    t2 = tcg_temp_new();
> +
> +    check_loongarch_64(ctx);
> +    tcg_gen_muls2_i64(t2, Rd, t0, t1);

If you actually supported LA32, you'd notice this doesn't compile.  Are you planning to 
support LA32 in the future?

> +static bool trans_lu32i_d(DisasContext *ctx, arg_lu32i_d *a)
> +{
> +    TCGv_i64 t0, t1;
> +    TCGv Rd = cpu_gpr[a->rd];
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    t0 = tcg_temp_new_i64();
> +    t1 = tcg_temp_new_i64();
> +
> +    tcg_gen_movi_tl(t0, a->si20);
> +    tcg_gen_concat_tl_i64(t1, Rd, t0);
> +    tcg_gen_mov_tl(Rd, t1);

Hmm.  Better as

  tcg_gen_deposit_tl(Rd, Rd, tcg_constant_tl(a->si20), 32, 32);

> +static bool trans_lu52i_d(DisasContext *ctx, arg_lu52i_d *a)
> +{
> +    TCGv t0, t1;
> +    TCGv Rd = cpu_gpr[a->rd];
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    t0 = tcg_temp_new();
> +    t1 = tcg_temp_new();
> +
> +    gen_load_gpr(t1, a->rj);
> +
> +    tcg_gen_movi_tl(t0, a->si12);
> +    tcg_gen_shli_tl(t0, t0, 52);
> +    tcg_gen_andi_tl(t1, t1, 0xfffffffffffffU);
> +    tcg_gen_or_tl(Rd, t0, t1);

Definitely better as

  tcg_gen_deposit_tl(Rd, Rd, tcg_constant_tl(a->si12), 52, 12);

> +static bool trans_addi_w(DisasContext *ctx, arg_addi_w *a)
> +{
> +    TCGv Rd = cpu_gpr[a->rd];
> +    TCGv Rj = cpu_gpr[a->rj];
> +    target_ulong uimm = (target_long)(a->si12);
> +
> +    if (a->rd == 0) {
> +        /* Nop */
> +        return true;
> +    }
> +
> +    if (a->rj != 0) {
> +        tcg_gen_addi_tl(Rd, Rj, uimm);
> +        tcg_gen_ext32s_tl(Rd, Rd);
> +    } else {
> +        tcg_gen_movi_tl(Rd, uimm);
> +    }
> +
> +    return true;
> +}

Again, there should be a common function for all of the two-register-immediate operations. 
 The callback here is exactly the same as for trans_add_w.

> +static bool trans_xori(DisasContext *ctx, arg_xori *a)
> +{
> +    TCGv Rd = cpu_gpr[a->rd];
> +    TCGv Rj = cpu_gpr[a->rj];
> +
> +    target_ulong uimm = (uint16_t)(a->ui12);

You shouldn't need these sorts of casts.


r~