Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Largest element index is vague #873

Open
jrahmeh opened this issue Apr 3, 2023 · 7 comments · May be fixed by #876
Open

Largest element index is vague #873

jrahmeh opened this issue Apr 3, 2023 · 7 comments · May be fixed by #876

Comments

@jrahmeh
Copy link

jrahmeh commented Apr 3, 2023

Section 3.7 says:

The use of vstart values greater than the largest element index for the current SEW setting is reserved.

Is the largest element index computed using the current group multiplier, or the largest group multiplier (8)?
For load and store operations with explicit element width (example: vle8.v), do we use the current SEW in vtype or the effective element width (8 for vle8.v)?

@hdelassus
Copy link
Contributor

This was fixed by #850 (emphasis mine):

The use of vstart values greater than the largest element index for the current vtype setting is reserved.

I think it would make sense to me if the EEW applied to the largest element index calculation for load/store ops with explicit element width.

@aswaterman
Copy link
Collaborator

aswaterman commented Apr 18, 2023

@hdelassus Yes, you're definitely right about that. Can you make a PR that makes the wording more precise? I think it needs to factor in not just EEW but also EMUL, right?

Actually, on further thought, isn't it correct as-is? Because the larger-EEW operands also have EMUL adjusted by the same factor, simply using SEW and LMUL (as the current text suggests) is equivalent to using EEW and EMUL for the widest type.

Whole-register loads and stores are a counterexample to the struckthrough text, since their EEW:EMUL ratio might not be equal to the SEW:LMUL ratio. I think you're right that "EEW and EMUL of the widest operand" is correct, but so would be "EEW and EMUL of the narrowest operand", since they have the same ratio for all standard instructions. Not sure what the best phrasing is.

@jrahmeh
Copy link
Author

jrahmeh commented Apr 18, 2023

How about: "The use of vstart values greater than the largest element index is reserved. The largest element index is determined using the effective element width and effective group multiplier of the current instruction."

@hdelassus
Copy link
Contributor

@hdelassus Yes, you're definitely right about that. Can you make a PR that makes the wording more precise? I think it needs to factor in not just EEW but also EMUL, right?

Sounds about right to me. Also, @jrahmeh's suggestion LGTM.

Whole-register loads and stores are a counterexample to the struckthrough text, since their EEW:EMUL ratio might not be equal to the SEW:LMUL ratio.

Also mask loads and stores, which have EEW=8 and EMUL=1, right?

@aswaterman
Copy link
Collaborator

Also mask loads and stores

Yes, those, too.

Would either of you mind making a PR?

@nick-knight
Copy link
Contributor

Not 100% sure, but we might need a similar modification to a subsequent statement:

Implementations are permitted to raise illegal instruction exceptions when attempting to execute a vector instruction with a value of vstart that the implementation can never produce when executing that same instruction with the same vtype setting.

I've always been a little confused about potential overlap between this statement and the earlier one being debated in this issue. So take my comment with a grain of salt.

@aswaterman
Copy link
Collaborator

@nick-knight I think that one should stay as-is. Implicitly, it admits the possibility that the constraints on vstart depend on the opcode, or on vtype, or on a combination of the opcode and vtype. So it covers all bases.

hdelassus added a commit to hdelassus/riscv-v-spec that referenced this issue Apr 18, 2023
@hdelassus hdelassus linked a pull request Apr 18, 2023 that will close this issue
hdelassus added a commit to hdelassus/riscv-v-spec that referenced this issue Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants