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

duplicates: xperm.n == xperm4, xperm.b == xperm8, rev.b == brev8 #1518

Closed
mjosaarinen opened this issue Jul 10, 2024 · 6 comments · Fixed by #1522
Closed

duplicates: xperm.n == xperm4, xperm.b == xperm8, rev.b == brev8 #1518

mjosaarinen opened this issue Jul 10, 2024 · 6 comments · Fixed by #1522

Comments

@mjosaarinen
Copy link
Contributor

mjosaarinen commented Jul 10, 2024

This can be confusing and I don't see why it would be intentional:

  • Sect 30.5.49. xperm.n and Sect 34.4.48. xperm4 seem to define exactly the same instruction with different names.
  • Also 30.5.48. xperm.b === 34.4.47. xperm8.
  • Also 30.5.30. rev.b == 34.4.13. brev8.

I recall that (Krste) requested us to change the names of { rev.b, xperm.n, xperm.b } from the bitmanip names in late stages of scalar crypto ratification -- just before the public review. So we changed those. I guess the old names made a comeback from the old bitmanip specification into the merged spec?

Here's the original issue that motivated the change: riscv/riscv-crypto#115

For scalar crypto the change happened for v1.0.0-rc2 in September 2021: https://github.com/riscv/riscv-crypto/releases/tag/v1.0.0-rc2-scalar

@mjosaarinen mjosaarinen changed the title dupplicates: xperm.n == xperm4, xperm.b == xperm8, rev.b == brev8 duplicates: xperm.n == xperm4, xperm.b == xperm8, rev.b == brev8 Jul 10, 2024
@aswaterman
Copy link
Member

The fundamental problem, IMO, is the duplication of the Zbkb, Zbkc, and Zbkx specs between the bitmanip and scalar crypto chapters. I think it makes the most sense to consolidate them into the bitmanip chapter, but fix the mnemonics to correspond to the ones in the scalar crypto chapter, as you suggest. If someone can make a PR that does this, I'd be happy to review it.

@wmat
Copy link
Collaborator

wmat commented Jul 11, 2024

I will take care of this PR.

@aswaterman
Copy link
Member

@mjosaarinen My PR fixes the mnemonics: #1524

Since no good deed goes unpunished, can you sanity-check Zbkb/Zbkc/Zbkx in the bitmanip chapter? I did some spot checks. The opcodes look right, but you might want to check them anyway. The SAIL code is differently formatted, though. (I didn't check its semantics.) If you have any recommendations, please follow up with a PR, which I'll be happy to review.

@mjosaarinen
Copy link
Contributor Author

Thanks,

Here are some things from the review:

There's a typo here:

xprem8 _rd_, _rs1_, _rs2_

As you note, it would be great if andn clmul clmulh orn pack packh packw rev8 rol rolw ror rori roriw rorw unzip xnor xperm4 xperm8 zip would not be defined twice in two different sections.

Typesetting of sail for brev8, xperm4, xperm8, in scalar-crypto.adoc is different from the one in b-st-ext.adoc as all indent has been removed from the crypto definitions of these instructions.

The sail definitions in bitmanip retain the old names in pseudocode; e.g. xpermn_lookup

val xpermn_lookup : (bits(4), xlenbits) -> bits(4)

Furthermore on the bitmanip version retains the old names in links.

|<<#insns-xpermn>>

Cheers,
-markku

@mjosaarinen
Copy link
Contributor Author

PR from the review #1531

aswaterman added a commit that referenced this issue Jul 15, 2024
Cleanup for of XPERM4, XPERM8, BREV8  ( #1518 )
@aswaterman
Copy link
Member

Many thanks. I'll consider this one resolved for now.

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 a pull request may close this issue.

3 participants