LLVM implements the 0.3 draft specification:
https://github.com/riscv/riscv-crypto/releases/download/v20230206/riscv-crypto-spec-vector.pdf
, and current vector crypto extension version can be found in:
https://github.com/riscv/riscv-crypto.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Since there's some accidentally pushed code. The old revision is moved to this one, sorry..
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
@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. |
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.
llvm/lib/Support/RISCVISAInfo.cpp | ||
---|---|---|
888 | 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. |
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.
llvm/lib/Support/RISCVISAInfo.cpp | ||
---|---|---|
893 | For consistency with the Zvk* error, use "'zvknhb' requires 'v' or 'zve64*' extension to also be specified" |
Line exceeds 80 characters