Re: [PATCH for 9.0 v15 04/10] target/riscv: always clear vstart in whole vec move insns

[Alistair Francis]

On Fri, Mar 15, 2024 at 3:58 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> These insns have 2 paths: we'll either have vstart already cleared if
> vstart_eq_zero or we'll do a brcond to check if vstart >= maxsz to call
> the 'vmvr_v' helper. The helper will clear vstart if it executes until
> the end, or if vstart >= vl.
>
> For starters, the check itself is wrong: we're checking vstart >= maxsz,
> when in fact we should use vstart in bytes, or 'startb' like 'vmvr_v' is
> calling, to do the comparison. But even after fixing the comparison we'll
> still need to clear vstart in the end, which isn't happening too.
>
> We want to make the helpers responsible to manage vstart, including
> these corner cases, precisely to avoid these situations:
>
> - remove the wrong vstart >= maxsz cond from the translation;
> - add a 'startb >= maxsz' cond in 'vmvr_v', and clear vstart if that
>   happens.
>
> This way we're now sure that vstart is being cleared in the end of the
> execution, regardless of the path taken.
>
> Fixes: f714361ed7 ("target/riscv: rvv-1.0: implement vstart CSR")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/insn_trans/trans_rvv.c.inc | 3 ---
>  target/riscv/vector_helper.c            | 5 +++++
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc 
> b/target/riscv/insn_trans/trans_rvv.c.inc
> index 8c16a9f5b3..52c26a7834 100644
> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> @@ -3664,12 +3664,9 @@ static bool trans_##NAME(DisasContext *s, arg_##NAME * 
> a)               \
>                               vreg_ofs(s, a->rs2), maxsz, maxsz);        \
>              mark_vs_dirty(s);                                           \
>          } else {                                                        \
> -            TCGLabel *over = gen_new_label();                           \
> -            tcg_gen_brcondi_tl(TCG_COND_GEU, cpu_vstart, maxsz, over);  \
>              tcg_gen_gvec_2_ptr(vreg_ofs(s, a->rd), vreg_ofs(s, a->rs2), \
>                                 tcg_env, maxsz, maxsz, 0, gen_helper_vmvr_v); 
> \
>              mark_vs_dirty(s);                                           \
> -            gen_set_label(over);                                        \
>          }                                                               \
>          return true;                                                    \
>      }                                                                   \
> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> index 34ac4aa808..bcc553c0e2 100644
> --- a/target/riscv/vector_helper.c
> +++ b/target/riscv/vector_helper.c
> @@ -5075,6 +5075,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState 
> *env, uint32_t desc)
>      uint32_t startb = env->vstart * sewb;
>      uint32_t i = startb;
>
> +    if (startb >= maxsz) {
> +        env->vstart = 0;
> +        return;
> +    }
> +
>      if (HOST_BIG_ENDIAN && i % 8 != 0) {
>          uint32_t j = ROUND_UP(i, 8);
>          memcpy((uint8_t *)vd + H1(j - 1),
> --
> 2.44.0
>
>