| Message ID | 20230706104433.16264-1-rbradford@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Fix LMUL check to use minimum SEW | expand |
On 7/6/23 07: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) > > Fixes: d9b7609a1fb2 ("target/riscv: rvv-1.0: configure instructions") > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com> > --- Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.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; > } > }
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) 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. 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; > } > }
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 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; > > } > > } >
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; >>> } >>> }
On 2023/7/18 8:43, Weiwei Li wrote: > > 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. Yes. Currently, we only check sew <= cpu.cfg->elen. We should also add a check SEW <= VLEN * LMUL Zhiwei > > 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; >>>> } >>>> }
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; } }
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) 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(-)