Re: [PATCH v2 03/18] target/riscv: Add packed SIMD arithmetic instructions

On 2026-07-17 10:06, Molly Chen wrote:
> Signed-off-by: Molly Chen <[email protected]>
> ---
>  target/riscv/helper.h                       |  41 ++
>  target/riscv/insn32.decode                  |  63 +++
>  target/riscv/tcg/insn_trans/trans_rvp.c.inc | 547 ++++++++++++++++++++
>  target/riscv/tcg/psimd_helper.c             | 114 ++++
>  target/riscv/tcg/translate.c                |   4 +
>  5 files changed, 769 insertions(+)
>  create mode 100644 target/riscv/tcg/insn_trans/trans_rvp.c.inc
...
> diff --git a/target/riscv/tcg/insn_trans/trans_rvp.c.inc b/target/riscv/tcg/insn_trans/trans_rvp.c.inc
> new file mode 100644
> index 00000000000..056ccfb486e
> --- /dev/null
> +++ b/target/riscv/tcg/insn_trans/trans_rvp.c.inc
...
> +#if defined(TARGET_RISCV32)
> +#define GEN_SIMD_TRANS_REG_PAIR_LANE_SCALAR_OP(INSN, HELPER, VXSAT)     \
> +static bool trans_##INSN(DisasContext *ctx, arg_##INSN * a)    \
> +{                                                              \
> +    REQUIRE_32BIT(ctx);                                        \
> +    REQUIRE_RVP(ctx);                                          \
> +    if (VXSAT && !prepare_rvp_vxsat(ctx)) {                    \
> +        return false;                                          \
> +    }                                                          \
> +    TCGv src1_0 = get_gpr(ctx, (a->rs1) * 2, EXT_NONE);        \
> +    TCGv dest_0 = dest_gpr(ctx, (a->rd) * 2);                    \
> +    TCGv src1_1 = get_gpr(ctx, (a->rs1) * 2 + 1, EXT_NONE);        \
> +    TCGv dest_1 = dest_gpr(ctx, (a->rd) * 2 + 1);                  \
> +    TCGv src2   = get_gpr(ctx, a->rs2, EXT_NONE);              \
> +    gen_helper_##HELPER(dest_0, tcg_env, src1_0, src2);        \
> +    gen_helper_##HELPER(dest_1, tcg_env, src1_1, src2);        \
> +    gen_set_gpr(ctx, (a->rd) * 2, dest_0);                     \
> +    gen_set_gpr(ctx, (a->rd) * 2 + 1, dest_1);                 \
> +    return true;                                              \
> +}

There is a read-after-write hazard on src2 when the destination pair
overlaps rs2.

get_gpr(*, EXT_NONE) returns cpu_gpr[n] itself (EXT_NONE takes no
snapshot). And dest_gpr(ctx, rd) also returns cpu_gpr[rd] itself.
So if rd*2 == a->rs2 (or rd*2 + 1 == a->rs2), dest_0/dest_1 and src2
are the same TCG global, and the first gen_helper call op writes it
before the second call op reads it as its source.

I suggest that we could fix it by snapshot rs2:

       TCGv dest_1 = dest_gpr(ctx, (a->rd) * 2 + 1);              \
  -    TCGv src2   = get_gpr(ctx, a->rs2, EXT_NONE);              \
  +    TCGv src2 = tcg_temp_new();                                \
  +    tcg_gen_mov_tl(src2, get_gpr(ctx, a->rs2, EXT_NONE));      \
       gen_helper_##HELPER(dest_0, tcg_env, src1_0, src2);        \

> +#if defined(TARGET_RISCV32)
> +#define GEN_SIMD_TRANS_PN_OP(NAME, SRC2, VXSAT)            \
> +static bool trans_##NAME(DisasContext *ctx, arg_##NAME * a) \
> +{                                                           \
> +    REQUIRE_32BIT(ctx);                                     \
> +    REQUIRE_RVP(ctx);                                       \
> +    if (VXSAT && !prepare_rvp_vxsat(ctx)) {                 \
> +        return false;                                       \
> +    }                                                       \
> +    TCGv_i64 s1 = tcg_temp_new_i64();                       \
> +    if (a->rs1 == 0) {                                      \
> +        tcg_gen_mov_i64(s1, 0);                             \

I think that we should use tcg_gen_movi_i64 here.

rnax

> +    } else {                                                \
> +        get_pair_regs(ctx, s1, a->rs1 * 2);                 \
> +    }                                                       \
> +    TCGv src2 = SRC2;                                       \
> +    TCGv dest = dest_gpr(ctx, a->rd);                       \
> +    gen_helper_##NAME(dest, tcg_env, s1, src2);             \
> +    gen_set_gpr(ctx, a->rd, dest);                          \
> +    return true;                                            \
> +}