This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support vector crypto extension ISA string and assembly
ClosedPublic

Authored by 4vtomat on Jan 13 2023, 2:52 AM.

Diff Detail

Event Timeline

4vtomat created this revision.Jan 13 2023, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2023, 2:52 AM
4vtomat requested review of this revision.Jan 13 2023, 2:52 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 13 2023, 2:52 AM
4vtomat added a reviewer: kito-cheng.

Since there's some accidentally pushed code. The old revision is moved to this one, sorry..

craig.topper added inline comments.Jan 19 2023, 9:50 AM
llvm/lib/Support/RISCVISAInfo.cpp
871

Line exceeds 80 characters

llvm/lib/Target/RISCV/RISCV.td
473 ↗(On Diff #488929)

Indented too much

480 ↗(On Diff #488929)

Indented too much

4vtomat updated this revision to Diff 493826.Jan 31 2023, 10:27 PM

NFC, some indentation fix.

reames added a subscriber: reames.Feb 2 2023, 2:26 PM

This was brought up in today's RISCV sync call; I want to summarize the major points of discussion.

This patch is now behind the current most recent revision on the vector-crypto spec. (https://github.com/riscv/riscv-crypto/releases/download/v20230125/riscv-crypto-spec-vector.pdf) The assumption is that the patch needs to be updated to match most recent. If there's a reason why this assumption is wrong, please explicitly describe the argument.

There was confusion around the versioning scheme used in the vector-crypto spec. It looks like newer version have moved to an internally consistent (and less than 1.0) version scheme, so I think this item is resolved.

There was a mention of another change being made to the spec in the near future and a desire to wait until that had happened. This isn't required by our experimental extension policy. However, in generally trying to minimize churn by delaying a couple days is not unreasonable. I'd welcome comment from someone following spec changes as to whether we should delay here or not, and why.

This was brought up in today's RISCV sync call; I want to summarize the major points of discussion.

This patch is now behind the current most recent revision on the vector-crypto spec. (https://github.com/riscv/riscv-crypto/releases/download/v20230125/riscv-crypto-spec-vector.pdf) The assumption is that the patch needs to be updated to match most recent. If there's a reason why this assumption is wrong, please explicitly describe the argument.

There was confusion around the versioning scheme used in the vector-crypto spec. It looks like newer version have moved to an internally consistent (and less than 1.0) version scheme, so I think this item is resolved.

There was a mention of another change being made to the spec in the near future and a desire to wait until that had happened. This isn't required by our experimental extension policy. However, in generally trying to minimize churn by delaying a couple days is not unreasonable. I'd welcome comment from someone following spec changes as to whether we should delay here or not, and why.

Here are some key points from today's crypto meeting that someone in attendance shared.

  • vghmac.vv
    • a new variant vghacm.vv will be added (xor then mul)
  • vandn operands will be reversed to match other insns in RISC-V
  • new constraint for vl and element group operations: if VLMAX < EGS (equivalently VLEN * LMUL < EGW): raise an illegal exception (on top of vl % EGS != 0)
  • Will define two meta extensions Zvkn and Zvks for the RVA23 profile
  • Zvkn = Zvkns + Zvknhb (AES, SHA-256, SHA-512)
  • Zvks = Zvksed + Zvksh (SM3, SM4)
  • Will ask to add Zvkg + Zvkb as optional for RVA23 profile
4vtomat updated this revision to Diff 498253.Feb 16 2023, 10:56 PM
4vtomat marked 2 inline comments as done.

Update to v20230206

4vtomat edited the summary of this revision. (Show Details)Feb 16 2023, 11:03 PM

@reames and I recently talked and we agreed that vector crypto is far enough along that we'd like to get something committed to LLVM in experimental state. We can do iterative refinement if the spec continues to change. I guess the question is whether we should update to the most recent now or land this first.

llvm/lib/Target/RISCV/RISCVFeatures.td
508

Indent this to line up with "'Zvknhb' on the previous line

537

This is indented 1 space too far.

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
11

Is this version out of date?

16

Can we name this VCLMUL_MV_V_X. That make it consistent with the similar VMUL_MV_V_X, VALU_MV_V_X, etc.

46

VROR_IV_V_X_I seems like a more consistent name.

75

I think this should be something like VAES_MV_V_S based on how the other multiclasses are named.

83

Maybe this should be something like VAESKF_MV_I?

llvm/test/MC/RISCV/rvv/zvkb.s
15

Add {{$}} to the end of the line

llvm/test/MC/RISCV/rvv/zvknh.s
23

Add {{$}} to the end of the line to make sure it checks the whole line. Same for all similar lines in all tests.

asb added a comment.Mar 8 2023, 2:16 AM

I guess the question is whether we should update to the most recent now or land this first.

I think updating to the most recent would make sense. Review bandwidth is limited, and it doesn't look like a huge amount has been expended on this particular version of the patch already. So it seems most efficient to update to the latest version and then request review. Perhaps there are specifics I'm not aware of that change the balance, but that would be my default view.

4vtomat updated this revision to Diff 504034.Mar 9 2023, 11:28 PM

Resolved Craig's comments and rename *zvkns* files to *zvkned*.

craig.topper added inline comments.Mar 13 2023, 3:53 PM
llvm/lib/Support/RISCVISAInfo.cpp
877

Put single quotes around extension names in the error messages. I fixed the other error messages last week

llvm/lib/Target/RISCV/RISCVFeatures.td
508

Zvknhb implies Zvknha here it needs to imply it in RISCVISAInfo.cpp too.

509

Line 495 has Zvknhb implying Zvknha which would make it sufficient to only check HasStdExtZknha. I'm not sure if this code should be changed or the implies at line 495.

4vtomat updated this revision to Diff 505112.Mar 14 2023, 8:24 AM

Resolved Craig's comments.

FYI, zvkb was discussed at the Architectural Review Committee this week and they've asked for some significant changes and expansions. See the notes shared here: https://lists.riscv.org/g/tech-unprivileged/message/450

I actively do not think we need to wait on landing this patch until those expansions have been finalized. I think is fine to land this in experimental state, and then transform zvkb into the requested form. Having a generic vector bitmanip extension pulled out seems to make a lot of sense to me, and I like (most of) the proposed additions as they close holes we've encountered in codegen.

I've skimmed the review and didn't spot anything glaring, but want to leave approval to @craig.topper since he's already done one pass and is generally more familiar with the MC layer than I am.

craig.topper added inline comments.Mar 15 2023, 3:10 PM
llvm/lib/Support/RISCVISAInfo.cpp
882

For consistency with the Zvk* error, use

"'zvknhb' requires 'v' or 'zve64*' extension to also be specified"

4vtomat updated this revision to Diff 506894.Mar 21 2023, 3:32 AM

Resolved Craig's comment.

This revision is now accepted and ready to land.Mar 21 2023, 7:00 PM