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

|Thanks for catching these too. I will fix these in the next version. 
Thanks, Molly|

在 2026/8/17 0:32, Max Chou 写道:
> 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;                                            \
>> +}