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

[Daniel Henrique Barboza]

On 3/13/24 19:36, Richard Henderson wrote:
> 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.

Hm, so it seems like the comparison is broken in the translation today as well 
...


> >       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.

We're going for v15. Might as well fix this too.

At this point I'll rename the series to "riscv: vector fixes" since there's
a lot of stuff going on.

>      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);

I'll do an extra patch with it. Thanks,

Daniel

> r~