search

riscv / riscv-isa-manual

RISC-V Instruction Set Manual
https://riscv.org/
Creative Commons Attribution 4.0 International
3.71k stars 643 forks source link

[vector crypto] Clarifying mandate for vector register index alignment to LMUL/EMUL #1653

Open nibrunieAtSi5 opened 2 months ago

nibrunieAtSi5 commented 2 months ago

This change request against RISC-V vector crypto specification is related to https://github.com/riscv-software-src/riscv-isa-sim/issues/1548: it aims at making the alignment mandate of vector register index clear for any vector crypto instruction manipulating element groups (vector or scalar element groups).

In the spike issue, @GuoShibo-cn noticed that the spike behavior for vector register index alignment was not consistent with the standard constraints for Vector instructions (namely the operand/destination vector register index must be EMUL aligned, else the behavior is reserved).

Current spike does not trap and execute vector crypto instruction from https://gist.github.com/nibrunie/80a00047dce935011614530d86a829e6 without complaining.

There is a PR open on spike to fix this https://github.com/riscv-software-src/riscv-isa-sim/pull/1815

This PR (https://github.com/riscv/riscv-isa-manual/pull/1653) addresses the same two points (a) and (b):

(a) the EMUL alignment of vector operand/destination (=EMUL for the vector element group operands and the destination) is specified in the main RVV spec; when a vector register group is not EMUL aligned, the behavior is listed as "reserved" and spike has implemented this check to trigger illegal instruction exception when the condition was not met (e.g.

(b) The vector crypto specification lists multiple times the intent to allow the vector register group for the scalar-element-group to have EMUL = EGW / VLEN (and to be aligned with this EMUL and not with the global LMUL associated with other vector operands/destination of the instruction). Looking at the current version of the spec (https://github.com/riscv/riscv-isa-manual/blob/1745bbf5549e519dea428fa05178b9fe0a0449f3/src/vector-crypto.adoc#element-groups) one can find:

In the case of the .vs instructions defined in this specification, vs2 holds a 128-bit scalar element group. For implementations with VLEN ≥ 128, vs2 refers to a single register.

and elsewhere

Scalar element groups will occupy at most a single register in application processors. However, in implementations where VLEN<128, they will occupy 2 (VLEN=64) or 4 (VLEN=32) registers.

and also

The .vs instructions require scalar element groups of EGW=128. On implementations with VLEN < 128, these scalar element groups will necessarily be formed across registers. This is different from most scalars in vector instructions that typically consume part of a single register.

nibrunie commented 1 month ago

This was presented during the crypto task group meeting of October 10th 2024. @kdockser / @mjosaarinen (and I need to find Luis and Richard's handles): could you take a look and share any feedback ?

lfiolhais commented 1 month ago

@nibrunie @nibrunieAtSi5 I'm at lfiolhais. I don't know Richard's handle. This LGTM, in fact from the original document I implicitly got these requirements. I'm glad the document is more explicit. Thanks Nicolas!

nibrunieAtSi5 commented 1 month ago

I am trying to reach out to @kdockser on the RISC-V slack. I would like for him to review this change before moving forward.

kdockser commented 1 month ago

I think that this is concise way of stating what is in Source/Destination overlap constraints. Therefore, I suggest that it is added as a non-normative note in that section. Perhaps it should be placed between the text and the table.

Also, the note:

These EMUL alignment constraints allow scalar element group operands to be allocated to any vector register (without need to be aligned to LMUL) for any implementation with VLEN >= EGW.

conflicts with the overlap constraints; the scalar element group cannot be allocated to any vector register.

Perhaps we could go with something like this:

Scalar element group operands do not need to be aligned to LMUL for any implementation with VLEN >= EGW.

nibrunieAtSi5 commented 1 month ago

@kdockser, thank you for the second suggestion on the note; the wording "any vector register" was indeed incorrect.

For the first part of your feedback, I am not sure I understand: I think this goes beyond Source/Destination overlap constraints, as this clarify what source indices and destination index can actually be (even without overlap).

After reconsidering, I agree that "LMUL constraints" may not be the best location for this clarification but I don't think "Source/Destination overlap constraints" is either.

nibrunieAtSi5 commented 1 month ago

@kdockser,

I can suggest to move my additions into a new constraint sub-section: "Source/Destination index alignment constraints" and add the mention:

The standard RVV vector register group index alignment constraints apply.

What do you think ?

kdockser commented 3 weeks ago

@nibrunie My point was that this is not a clarification as it is already stated in "Source/Destination overlap constraints". Therefore, it should be a non-normative note. I agree that this note should be elsewhere.

Also, we need to make it explicit that Vector Crypto inherits all constraints from RVV.

nibrunieAtSi5 commented 3 weeks ago

@kdockser, I have made a few changes:

Adding an introduction to the instruction constraints section to enforce your recommendation:

Also, we need to make it explicit that Vector Crypto inherits all constraints from RVV.

==== Instruction Constraints
+
+All standard vector instruction constraints specified by RVV 1.0 apply to Vector Crypto instructions.
+In addition to those constraints a few additional specific constraints are introduced.

Moved my other additions to a new sub-section:

+EMUL and Source/Destination index alignment constraints::
++
+A *Vector Element Group* operand or destination has `EMUL = LMUL`.
++
+A *Scalar Element Group* operand or destination has `EMUL = ceil(EGW / VLEN)`.
++
+The https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#342-vector-register-grouping-vlmul20[Standard RVV vector register group alignment constraints] apply.
+[NOTE]
+====
+Scalar element group operands do not need to be aligned to LMUL for any implementation with VLEN >= EGW.
+====

Let me know what you think.

kdockser commented 3 weeks ago

@nibrunie I changed the section on overlapping registers into a section on .sv instructions. I also removed redundant statements about following the constraints of portions of RVV because you had added the statement that we were following all of the constraints of RVV. I'm still not quite sure how to propose changes on top of changes in github. Hopefully what I tried worked (and didn't mess anything up).

nibrunieAtSi5 commented 2 weeks ago

@nibrunie I changed the section on overlapping registers into a section on .sv instructions. I also removed redundant statements about following the constraints of portions of RVV because you had added the statement that we were following all of the constraints of RVV. I'm still not quite sure how to propose changes on top of changes in github. Hopefully what I tried worked (and didn't mess anything up).

Thank you @kdockser, the changes look good to me.

There are several way to suggest changes on top of a PR, one of them is to open a second PR which uses the branch of the first PR as the base branch (master is not the only base branch which can be used in a PR). But pushing directly on a branch is also a good way to do that. I would just recommend trying to avoid merging in master to keep the PR history linear (rebase should be preferred if the branch must be updated with master but this will rewrite the history and force all other participants to update locally in a not fast-forward fashion).

nibrunieAtSi5 commented 6 hours ago

@kdockser do you approve the current version of this PR ?