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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nibrunieAtSi5
Copy link
Contributor

@nibrunieAtSi5 nibrunieAtSi5 commented Sep 26, 2024

This change request against RISC-V vector crypto specification is related to riscv-software-src/riscv-isa-sim#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 riscv-software-src/riscv-isa-sim#1815

This PR (#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.

src/vector-crypto.adoc Outdated Show resolved Hide resolved
src/vector-crypto.adoc Outdated Show resolved Hide resolved
src/vector-crypto.adoc Outdated Show resolved Hide resolved
Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

LGTM but I'll wait for the TG to affirm.

src/vector-crypto.adoc Outdated Show resolved Hide resolved
Signed-off-by: Nicolas Brunie <[email protected]>
src/vector-crypto.adoc Outdated Show resolved Hide resolved
Signed-off-by: Nicolas Brunie <[email protected]>
@nibrunie
Copy link

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
Copy link

@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
Copy link
Contributor Author

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.

src/vector-crypto.adoc Outdated Show resolved Hide resolved
Signed-off-by: Nicolas Brunie <[email protected]>
@kdockser
Copy link
Collaborator

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
Copy link
Contributor Author

@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.

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