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

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

On 11/19/21 7:13 AM, Song Gao wrote:
>   DEF_HELPER_FLAGS_1(bitswap, TCG_CALL_NO_RWG_SE, tl, tl)
> +
> +DEF_HELPER_3(asrtle_d, void, env, tl, tl)
> +DEF_HELPER_3(asrtgt_d, void, env, tl, tl)

Use DEF_HELPER_FLAGS_3 and TCG_CALL_NO_WG.

(They do not write globals, but do implicitly read them via the exception path, and of 
course the exception is a side-effect.)

> +static bool gen_load_gt(DisasContext *ctx, arg_rrr *a, MemOp mop)
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
> +    TCGv src2 = gpr_src(ctx, a->rk, EXT_NONE);
> +    TCGv addr = tcg_temp_new();
> +
> +    gen_helper_asrtgt_d(cpu_env, src1, src2);
> +    tcg_gen_add_tl(addr, src1, src2);
> +    tcg_gen_qemu_ld_tl(dest, addr, ctx->mem_idx, mop);

This add is incorrect.  The address is rj (src1).
Likewise for the rest of the bound check memory ops.

> +static bool gen_ldptr(DisasContext *ctx, arg_rr_i *a, MemOp mop)
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv addr = gpr_src(ctx, a->rj, EXT_NONE);
> +    TCGv temp = NULL;
> +
> +    if (a->imm) {
> +        temp = tcg_temp_new();
> +        tcg_gen_addi_tl(temp, addr, a->imm << 2);
> +        addr = temp;
> +    }

I think it would be cleaner to have this immediate shift handled by decodetree, so that 
gen_ldptr is dropped in favor of gen_load (and also for stores).  It would also print the 
correct logical offset on the disassembly side.

%i14s2      10:s14 !function=shl_2
@rr_i14s2   .... .... .............. rj:5 rd:5  &rr_i imm=%i14s2


> +/* loongarch sync */
> +static void gen_loongarch_sync(int stype)
> +{
> +    TCGBar tcg_mo = TCG_BAR_SC;
> +
> +    switch (stype) {
> +    case 0x4: /* SYNC_WMB */
> +        tcg_mo |= TCG_MO_ST_ST;
> +        break;
> +    case 0x10: /* SYNC_MB */
> +        tcg_mo |= TCG_MO_ALL;
> +        break;
> +    case 0x11: /* SYNC_ACQUIRE */
> +        tcg_mo |= TCG_MO_LD_LD | TCG_MO_LD_ST;
> +        break;
> +    case 0x12: /* SYNC_RELEASE */
> +        tcg_mo |= TCG_MO_ST_ST | TCG_MO_LD_ST;
> +        break;
> +    case 0x13: /* SYNC_RMB */
> +        tcg_mo |= TCG_MO_LD_LD;
> +        break;
> +    default:
> +        tcg_mo |= TCG_MO_ALL;
> +        break;
> +    }
> +
> +    tcg_gen_mb(tcg_mo);
> +}

This is copied from mips, I think.  The only defined hint for dbar is 0.  I think this 
function should be removed, and just emit the tcg barrier directly within trans_dbar.


r~