Re: [PATCH] target/riscv: Fix LMUL check to use minimum SEW

[Weiwei Li]

On 2023/7/17 23:13, Rob Bradford wrote:
> On Thu, 2023-07-06 at 21:22 +0800, Weiwei Li wrote:
> > On 2023/7/6 18:44, Rob Bradford wrote:
> > > The previous check was failing with:
> > >
> > > ELEN = 64 SEW = 16 and LMUL = 1/8 (encoded as 5) which is a valid
> > > combination.
> > >
> > > Fix the check to correctly match the specification by using minimum
> > > SEW
> > > rather than the active SEW.
> > >
> > >   From the specification:
> > >
> > > "In general, the requirement is to support LMUL ≥ SEWMIN/ELEN,
> > > where
> > > SEWMIN is the narrowest supported SEW value and ELEN is the widest
> > > supported SEW value. In the standard extensions, SEWMIN=8. For
> > > standard
> > > vector extensions with ELEN=32, fractional LMULs of 1/2 and 1/4
> > > must be
> > > supported. For standard vector extensions with ELEN=64, fractional
> > > LMULs
> > > of 1/2, 1/4, and 1/8 must be supported."
> > >
> > >   From inspection this new check allows:
> > >
> > > ELEN=64 1/2, 1/4, 1/8 (encoded as 7, 6, 5 respectfully)
> > > ELEN=32 1/2, 1/4 (encoded as 7 and 6 respectfully)
> Hi Weiwei Li,
>
> Thanks for your reply. Sorry for delay in replying i've been away.
>
> > This is a little confusing.  there is  note in spec to explain why
> > LMUL
> > ≥ SEW MIN /ELEN:
> >
> > "When LMUL < SEW MIN /ELEN, there is no guarantee an implementation
> > would have enough bits in the fractional vector register to store
> >
> > Note at least one element, as VLEN=ELEN is a valid implementation
> > choice. For example, with VLEN=ELEN=32, and SEW MIN =8, an LMUL of
> >
> > 1/8 would only provide four bits of storage in a vector register."
> >
> > In this way, when VLEN=ELEN=64,  an LMUL of 1/8 would only provide 8
> > bits of storage in a vector register, so it's also not suitable for
> > sew
> > = 16.
> >
> > Maybe we can explain the above description of the spec in another
> > way:
> > we must support lmul=1/8 when ELEN=64, but it's only available when
> > sew = 8.
> I'm afraid i'm not sure I agree with this comment.
>
> VLEN=128 ELEN=64 SEW=16 LMUL=1/8 is a perfectly reasonable
> configuration and contradicts your statement.
>
> The goal of my patch was to ensure that we permit a valid configuration
> not to also reject other invalid configurations.
>
> An extra check that takes into consideration VLEN would also make sense
> to me:
>
> e.g. VLEN=64 LMUL=1/8 SEW=16 should be rejected

Yeah. I agree. But instead of an extra check, I think VLEN is the one 
that really works instead of ELEN.

Such as when ELEN=32,  LMUL=1/8 with SEW=8 is also a reasonable 
configuration  if VLEN >= 64.

Regards,

Weiwei Li

> Cheers,
>
> Rob
>
> > Regards,
> >
> > Weiwei Li
> >
> > `
> >
> > Regards,
> >
> > Weiwei Li
> >
> > > Fixes: d9b7609a1fb2 ("target/riscv: rvv-1.0: configure
> > > instructions")
> > >
> > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > ---
> > >    target/riscv/vector_helper.c | 4 ++--
> > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/riscv/vector_helper.c
> > > b/target/riscv/vector_helper.c
> > > index 1e06e7447c..8dfd8fe484 100644
> > > --- a/target/riscv/vector_helper.c
> > > +++ b/target/riscv/vector_helper.c
> > > @@ -43,9 +43,9 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env,
> > > target_ulong s1,
> > >                                                xlen - 1 -
> > > R_VTYPE_RESERVED_SHIFT);
> > >    
> > >        if (lmul & 4) {
> > > -        /* Fractional LMUL. */
> > > +        /* Fractional LMUL - check LMUL >= ELEN/SEW_MIN (8) */
> > >            if (lmul == 4 ||
> > > -            cpu->cfg.elen >> (8 - lmul) < sew) {
> > > +            cpu->cfg.elen >> (8 - lmul) < 8) {
> > >                vill = true;
> > >            }
> > >        }