Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polishing to the specification #271

Merged
merged 28 commits into from
Oct 13, 2023
Merged

Polishing to the specification #271

merged 28 commits into from
Oct 13, 2023

Conversation

eopXD
Copy link
Collaborator

@eopXD eopXD commented Sep 14, 2023

No description provided.

- Fix grammar and programming notes
- Adjust snippet for examples of overlodading exceptions
- Adjust wordings in multiple spot
- Unify ASCIIDoc link in the specification
- Add note that operand mnemonics `vi` variants don't exist on purpose

Signed-off-by: eop Chen <[email protected]>
Copy link
Collaborator

@nick-knight nick-knight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eopXD for all your improvements to the intrinsics documentation.

I haven't read through it all, I'm just responding to your change regarding vstart, since this came up in a separate conversation (about why the intrinsics API allows for undisturbed elements in vslideup but not vslidedown).

doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
@dzaima
Copy link

dzaima commented Sep 15, 2023

Do I understand correctly that the intent of a __riscv_vsetvl_* intrinsic is to be exactly min(avl, VLMAX), and thus be impossible to map directly to a vsetvli rd, rs1, ... instruction without some additional computation of the min()?

If that's the case, then the current LLVM and GCC implementations are non-conforming, as they do map directly to vsetvli: https://godbolt.org/z/bzYG3vxzM.

For example, on a VLEN=128 implementation (i.e. VLMAX=4 for e32m1), a wrap(6) call of the LLVM/GCC output would result in a vsetvli instruction that is allowed to, on a RVV1.0-compliant RISC-V implementation, produce a vl of either 3 or 4, as per "ceil(AVL / 2) ≤ vl ≤ VLMAX if AVL < (2 * VLMAX)" of section 6.3 of the V spec, and thus wrap(6) could equal 3, while this document specifies min(avl,VLMAX) = min(6,4) = 4.

Added autowidth, header options to tables.
Centered tables.
@eopXD
Copy link
Collaborator Author

eopXD commented Sep 15, 2023

Do I understand correctly that the intent of a __riscv_vsetvl_* intrinsic is to be exactly min(avl, VLMAX), and thus be impossible to map directly to a vsetvli rd, rs1, ... instruction without some additional computation of the min()?

If that's the case, then the current LLVM and GCC implementations are non-conforming, as they do map directly to vsetvli: https://godbolt.org/z/bzYG3vxzM.

For example, on a VLEN=128 implementation (i.e. VLMAX=4 for e32m1), a wrap(6) call of the LLVM/GCC output would result in a vsetvli instruction that is allowed to, on a RVV1.0-compliant RISC-V implementation, produce a vl of either 3 or 4, as per "ceil(AVL / 2) ≤ vl ≤ VLMAX if AVL < (2 * VLMAX)" of section 6.3 of the V spec, and thus wrap(6) could equal 3, while this document specifies min(avl,VLMAX) = min(6,4) = 4.

Ah, yes. You have found the hidden piece of the intrinsic implementation. However if you see this snippet https://godbolt.org/z/Wx4TPq7fs, the compiler is not respecting the element width and length multiplier provided by vsetvl. The vsetvl intrinsic is not intended to map to an exact instruction, the compiler is responsible to insert the appropriate vsetvl instruction.

Changed pdf-style attribute to pdf-theme as pdf-style was
deprecated.
@topperc
Copy link
Collaborator

topperc commented Sep 15, 2023

Do I understand correctly that the intent of a __riscv_vsetvl_* intrinsic is to be exactly min(avl, VLMAX), and thus be impossible to map directly to a vsetvli rd, rs1, ... instruction without some additional computation of the min()?
If that's the case, then the current LLVM and GCC implementations are non-conforming, as they do map directly to vsetvli: https://godbolt.org/z/bzYG3vxzM.
For example, on a VLEN=128 implementation (i.e. VLMAX=4 for e32m1), a wrap(6) call of the LLVM/GCC output would result in a vsetvli instruction that is allowed to, on a RVV1.0-compliant RISC-V implementation, produce a vl of either 3 or 4, as per "ceil(AVL / 2) ≤ vl ≤ VLMAX if AVL < (2 * VLMAX)" of section 6.3 of the V spec, and thus wrap(6) could equal 3, while this document specifies min(avl,VLMAX) = min(6,4) = 4.

Ah, yes. You have found the hidden piece of the intrinsic implementation. However if you see this snippet https://godbolt.org/z/Wx4TPq7fs, the compiler is not respecting the element width and length multiplier provided by vsetvl. The vsetvl intrinsic is not intended to map to an exact instruction, the compiler is responsible to insert the appropriate vsetvl instruction.

The compiler is still required respect the VLMAX implied by the SEW and LMUL. This means the compiler must respect the ratio between SEW and LMUL given to the intrinsic. The intrinsic must return the value the hardware vsetvli intruction would return for that VLMAX which is not always min(avl, VLMAX).

@dzaima
Copy link

dzaima commented Sep 15, 2023

Right, I am not saying that __riscv_vsetvl_e32m1 should necessarily map to vsetvli rd, rs1, e32, m1; indeed, vsetvli rd, rs1, e32, m1 and vsetvli rd, rs1, e64, m2 will result in the same rd value, and thus a compiler can choose to use those interchangeably for calculating rd.

What I am saying is that __riscv_vsetvl_e32m1(avl), if specified to be min(avl, VLMAX), can not be implemented as vsetvli rd, x[avl], e32, m1 (or ..., e64, m1, etc), even if the compiler wanted to do so, because the behavior of that vsetvli instruction is not guaranteed to be min(avl, VLMAX), as per the aforementioned section 6.3. of the V extension 1.0 specification.

Copy link
Collaborator

@nick-knight nick-knight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions related to the vl issues.

doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
@eopXD eopXD force-pushed the eopc/edit-spec branch 2 times, most recently from 07c6a2c to ad5d96f Compare September 18, 2023 11:18
@eopXD
Copy link
Collaborator Author

eopXD commented Sep 18, 2023

@nick-knight @dzaima @topperc Just made an update to description associated with "control of vl" and pseudo intrinsics vsetvl and vsetvlmax. Please check again, thank you.

@eopXD eopXD requested a review from nick-knight September 18, 2023 11:19
- Update description regarding control of number of elements to be
processed based on discussion threads and edit suggestions of Nick.
- Update descriptions of pseudo intrinsics vsetvl and vsetvlmax
- Update sub-topics to control of vector programming model to more
understanding phrases instead of the mnemonics of those CSRs.
- Use `argument` universally when describing intrinsics argments
instead of `operand` or `parameter`.
- Use `intrinsics` universally instead of `RVV C intrinsics` or
`intrinsics API`.
- Refind wordings more.
- Fix grammatical errors across the whole specification based on
suggestions by ChatGPT.

Signed-off-by: eop Chen <[email protected]>
Also, minor grammitical error fixes are included.

Signed-off-by: eop Chen <[email protected]>
@eopXD eopXD changed the title More polishing to the specification Polishing to the specification Sep 19, 2023
@eopXD
Copy link
Collaborator Author

eopXD commented Sep 25, 2023

Ping.

This will prevent the Appendices from appearing in the TOC as Chapters
but rather as Appendix A, Appendix B, etc.

Signed-off-by: eop Chen <[email protected]>
Load / store intrinsics follow the name under https://github.com/riscv/riscv-v-spec/blob/master/vmem-format.adoc
Other intrinsics follow the name under https://github.com/riscv/riscv-v-spec/blob/master/valu-format.adoc
and how they are defined under the vector specification.

Signed-off-by: eop Chen <[email protected]>
@eopXD
Copy link
Collaborator Author

eopXD commented Oct 3, 2023

Ping for more review comments, if not, the pull request will be merged one week later since we have converged to what this pull request is proposing.

Copy link
Collaborator

@rofirrim rofirrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eopXD polishing the document.

I left some comments, none of them too critical.

doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
doc/rvv-intrinsic-spec.adoc Outdated Show resolved Hide resolved
eopXD added 2 commits October 12, 2023 20:42
- Rephrase to be general
- Adjust description for compiler optimization emitting .vi mnemonic
  instructions

Signed-off-by: eop Chen <[email protected]>
Copy link
Collaborator

@nick-knight nick-knight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think there is room for further wordsmithing, I think the technical content of the documentation is sufficient to present this to the Software HC.

@eopXD
Copy link
Collaborator Author

eopXD commented Oct 13, 2023

Merging this PR now. Thanks all people that helped to work this through!

@eopXD eopXD merged commit c10de53 into main Oct 13, 2023
6 of 7 checks passed
@eopXD eopXD deleted the eopc/edit-spec branch October 23, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants