Re: [PATCH 11/13] trans_rvv.c.inc: remove vlmax arg from vec_element_loadx()

[Daniel Henrique Barboza]

On 1/12/24 19:51, Richard Henderson wrote:
> On 1/13/24 08:38, Daniel Henrique Barboza wrote:
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/insn_trans/trans_rvv.c.inc | 26 +++++++++++++++++--------
> >   1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/riscv/insn_trans/trans_rvv.c.inc 
> > b/target/riscv/insn_trans/trans_rvv.c.inc
> > index 804cfd6c7f..3782d0fa2f 100644
> > --- a/target/riscv/insn_trans/trans_rvv.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> > @@ -3265,21 +3265,28 @@ static void endian_adjust(TCGv_i32 ofs, int sew)
> >   #endif
> >   }
> > -/* Load idx >= VLMAX ? 0 : vreg[idx] */
> > +/*
> > + * Load idx >= VLMAX ? 0 : vreg[idx]
> > + *
> > + * This function assumes ctx->vl_eq_vlmax = true.
> > + */
> >   static void vec_element_loadx(DisasContext *s, TCGv_i64 dest,
> > -                              int vreg, TCGv idx, int vlmax)
> > +                              int vreg, TCGv idx)
>
> I think removing the cpu configuration constant is a mistake.
> Compile-time constants are always better than computation...

Apparently my commit msg is AWOL ...

The 'vlmax' used in vec_element_loadx() is being calculated here, in 
trans_vrgather_vx():

-        int scale = s->lmul - (s->sew + 3);
-        int vlmax = s->cfg_ptr->vlen >> -scale;

My idea was to eliminate the use of 'vlen' since, in this block, 'vl_eq_vlmax'
is true and we have 'vl' in the TCG global 'cpu_vl'.

I didn't find a way of reading 'cpu_vl' into an int variable and passing it as
'vlmax' to vec_element_loadx(), but inside vec_element_loadx() we can operate
'cpu_vl' normally since we're using TCG ops there. This is the reasoning behind
this change.

I am now wondering if this is worth the trouble, and we should instead do:

+    int vlmax = cpu->cfg.vlenb >> (s->sew - s->lmul);

Like we're already doing in patch 9. Patch 12 would be a similar case.



Thanks,

Daniel



> > +#ifdef TARGET_RISCV64
> > +    tcg_gen_mov_i64(t_vlmax, cpu_vl);
> > +#else
> > +    tcg_gen_extu_tl_i64(t_vlmax, cpu_vl);
> > +#endif
>
> That said, no ifdef required -- the second statement should always work.
>
>
>
> r~