* [PATCH] target/riscv: Fix LMUL check to use minimum SEW
@ 2023-07-06 10:44 Rob Bradford
2023-07-06 11:50 ` Daniel Henrique Barboza
2023-07-06 13:22 ` Weiwei Li
0 siblings, 2 replies; 6+ messages in thread
From: Rob Bradford @ 2023-07-06 10:44 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, Rob Bradford, Palmer Dabbelt, Alistair Francis,
Bin Meng, Weiwei Li, Daniel Henrique Barboza, Liu Zhiwei
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(-)
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;
}
}
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: Fix LMUL check to use minimum SEW
2023-07-06 10:44 [PATCH] target/riscv: Fix LMUL check to use minimum SEW Rob Bradford
@ 2023-07-06 11:50 ` Daniel Henrique Barboza
2023-07-06 13:22 ` Weiwei Li
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2023-07-06 11:50 UTC (permalink / raw)
To: Rob Bradford, qemu-devel
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng,
Weiwei Li, Liu Zhiwei
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;
> }
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: Fix LMUL check to use minimum SEW
2023-07-06 10:44 [PATCH] target/riscv: Fix LMUL check to use minimum SEW Rob Bradford
2023-07-06 11:50 ` Daniel Henrique Barboza
@ 2023-07-06 13:22 ` Weiwei Li
2023-07-17 15:13 ` Rob Bradford
1 sibling, 1 reply; 6+ messages in thread
From: Weiwei Li @ 2023-07-06 13:22 UTC (permalink / raw)
To: Rob Bradford, qemu-devel
Cc: liweiwei, qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng,
Daniel Henrique Barboza, Liu Zhiwei
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;
> }
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: Fix LMUL check to use minimum SEW
2023-07-06 13:22 ` Weiwei Li
@ 2023-07-17 15:13 ` Rob Bradford
2023-07-18 0:43 ` Weiwei Li
0 siblings, 1 reply; 6+ messages in thread
From: Rob Bradford @ 2023-07-17 15:13 UTC (permalink / raw)
To: Weiwei Li, qemu-devel
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng,
Daniel Henrique Barboza, Liu Zhiwei
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;
> > }
> > }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: Fix LMUL check to use minimum SEW
2023-07-17 15:13 ` Rob Bradford
@ 2023-07-18 0:43 ` Weiwei Li
2023-07-18 2:16 ` LIU Zhiwei
0 siblings, 1 reply; 6+ messages in thread
From: Weiwei Li @ 2023-07-18 0:43 UTC (permalink / raw)
To: Rob Bradford, qemu-devel
Cc: liweiwei, qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng,
Daniel Henrique Barboza, Liu Zhiwei
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;
>>> }
>>> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: Fix LMUL check to use minimum SEW
2023-07-18 0:43 ` Weiwei Li
@ 2023-07-18 2:16 ` LIU Zhiwei
0 siblings, 0 replies; 6+ messages in thread
From: LIU Zhiwei @ 2023-07-18 2:16 UTC (permalink / raw)
To: Weiwei Li, Rob Bradford, qemu-devel
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Bin Meng,
Daniel Henrique Barboza
[-- Attachment #1: Type: text/plain, Size: 4031 bytes --]
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;
>>>> }
>>>> }
[-- Attachment #2: Type: text/html, Size: 7402 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-18 2:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-06 10:44 [PATCH] target/riscv: Fix LMUL check to use minimum SEW Rob Bradford
2023-07-06 11:50 ` Daniel Henrique Barboza
2023-07-06 13:22 ` Weiwei Li
2023-07-17 15:13 ` Rob Bradford
2023-07-18 0:43 ` Weiwei Li
2023-07-18 2:16 ` LIU Zhiwei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.