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

zvk: Check VR index alignment constraint and vm constraint #1843

Closed
wants to merge 1 commit into from

Conversation

YenHaoChen
Copy link
Collaborator

The vector specification requires aligning the VR index regarding the LMUL. Also, it requires that vd cannot be v0 when masking.

image

image

Although the vector crypto specification does not explicitly state those two constraints, the deriving extension should include the general constraints from the vector specification.

This commit checks the VR index and vm constraints on Zvk instructions.

The vector specification requires aligning the VR index regarding the
LMUL. Also, it requires that vd cannot be v0 when masking.

Although the vector crypto specification does not explicitly state those
two constraints, the deriving extension should include the general
constraints from the vector specification.

This commit checks the VR index and vm constraints on Zvk instructions.
@aswaterman
Copy link
Collaborator

@nibrunieAtSi5 @kdockser

@nibrunieAtSi5
Copy link
Contributor

I think I tried to cover some of the missing alignment constraints in #1815.

I don't think this PR handles the scalar-element-group source register index alignment for .vs the way it should be (see riscv/riscv-isa-manual#1653): the vs2 operand alignment should not be checked against LMUL.

None of the vector crypto instruction working on element groups support masking.

@nibrunieAtSi5
Copy link
Contributor

@YenHaoChen , could you help review #1815 ? It should fix the issues you intended to fix there (excluding the mask overlap which I believe is not an issue for any instruction working with element groups in vector crypto).

@YenHaoChen YenHaoChen closed this Oct 25, 2024
@YenHaoChen YenHaoChen deleted the pr-zvk branch October 25, 2024 05:12
@YenHaoChen YenHaoChen restored the pr-zvk branch October 25, 2024 05:12
@YenHaoChen
Copy link
Collaborator Author

YenHaoChen commented Oct 25, 2024

@nibrunieAtSi5
Before reviewing the PR, I need to familiarize myself with the vector crypto specification. Nevertheless, the behavior seems correct at a glance.

There are non-behavioral concerns in #1815. Reviewers would be much more willing to approve the PR if it could make the following modifications.

  1. Usually, we expect each PR to deal with one thing. The PR implements two things. Perhaps, we can split it into two PRs, and reviewers could approve them separately.
  2. Reviewers would be thankful if the contributor could squash and rebase their PR. Specifically, the second commit deletes all the codes from the first commit and adds different codes, confusing the reviewers. We may delete the first commit to help reviewers understand the PR. (Does the later commit delete all the code from the second commit again? I am unsure which commit contributes to the final code.)
  3. Comments should go along with the code. Commits 61ad5bd and 0f1ef5c modify the comments only. Please squash the modification to the corresponding commits.
  4. PR should not include typo correction of the PR itself. Please squash commits 636a5f3 and 494e219.

@kdockser
Copy link

As I just mentioned in another discussion. We need to make a blanket statement that Vector Crypto inherits all constraints from RVV.

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.

4 participants