Re: [PATCH for-9.0 v14 3/8] target/riscv: always clear vstart in whole vec move insns

[Richard Henderson]

On 3/13/24 12:01, Daniel Henrique Barboza 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.
>
> However, if vstart >= maxsz, the helper will be skipped, and vstart
> won't be cleared since the helper is being responsible from doing it.
>
> We want to make the helpers responsible to manage vstart, including
> these corner cases, precisely to avoid these situations. Move the vstart
> > = maxsz cond to the helper, and be sure to 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>
> ---
>   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 ca79571ae2..cd8235ea98 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 (env->vstart >= maxsz) {
> +        env->vstart = 0;
> +        return;
> +    }

I think you were right to be concerned about vlmax -- you need to use startb not vstart in 
this comparison, otherwise you've got mixed units on the comparison.

>       memcpy((uint8_t *)vd + H1(i),
>              (uint8_t *)vs2 + H1(i),
>              maxsz - startb);

Unrelated to this patch series, this has a big-endian host error.
With big-endian, the bytes to be copied may not be sequential.

    if (HOST_BIG_ENDIAN && i % 8 != 0) {
        uint32_t j = ROUND_UP(i, 8);
        memcpy(vd + H(j - 1), vs2 + H1(j - 1), j - i);
        i = j;
    }
    memcpy(vd + i, vs2 + i, maxsz - i);


r~