Page MenuHomePhabricator

[RISCV][RFC] Initially support the K-extension instructions on the LLVM MC layer
Needs ReviewPublic

Authored by VincentWu on Mar 6 2021, 10:05 PM.

Details

Summary

This commit is currently implementing supports for scalar cryptography extension for LLVM according to version 0.9.0 (Pre-release) and version 0.8.1 of K Ext specification. Currently, we are implementing the MC (Machine Code) layer of his extension and the majority of work is done under llvm/lib/Target/RISCV directory. There are also some test files in llvm/test/MC/RISCV/rvk directory.

Add two new CSR, MNOISE and MENTROPY.
Add two new Immediate numbers, uimm2 and rcon.

Co-author:@ksyx & @VincentWu

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
VincentWu requested review of this revision.Mar 6 2021, 10:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2021, 10:05 PM
VincentWu updated this revision to Diff 328838.Mar 6 2021, 10:25 PM
VincentWu edited the summary of this revision. (Show Details)

refactor: move the definition of the immediate number uimm2 and uimm4

VincentWu updated this revision to Diff 328840.Mar 6 2021, 10:50 PM

fix: fix error trailing whitespace when building

craig.topper retitled this revision from [RFC] Initially support the K-extension instructions on the LLVM MC layer to [RISCV][RFC] Initially support the K-extension instructions on the LLVM MC layer.Mar 6 2021, 11:10 PM
craig.topper added inline comments.Mar 6 2021, 11:14 PM
llvm/lib/Target/RISCV/RISCVInstrInfoK.td
10

Is this supposed to say RISC-V instead of RISC-K?

VincentWu updated this revision to Diff 328842.Mar 6 2021, 11:27 PM

typo: RISC-K ->RISC-V

jrtc27 added a comment.Mar 7 2021, 8:00 AM

Hm, having SHA-2 acceleration but not SHA-3 in 2021 is a bit surprising to me

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
506

Blank line before

514

Blank line after

llvm/lib/Target/RISCV/RISCV.td
196

Whitespace

llvm/lib/Target/RISCV/RISCVInstrInfoK.td
20

The formatting for this file is (a) messy (not consistent with itself) and (b) does not match the other extensions. Please reformat the entire thing to conform with the existing style.

46

Isn't this the round, which is then used to derive the 8-bit round constant, not the round constant itself?

57

These comments are pointless

59

If you don't have scheduling defined yet just don't add Sched<[]> at all?

75
Assembler Pseudo Instructions

to distinguish from LLVM Pseudos.

76

Naming aliases is pointless

137

Should be fixed

llvm/lib/Target/RISCV/RISCVSubtarget.h
55

Be consistent whether it's "V K Zv*" or "V Zv* K" (latter being what you've done in TableGen). I think I prefer grouping all the V stuff together even if technically Z* extensions sort after all the single-letter ones.

Also, you will need to get the canonical order updated to include K at some point, though it seems relatively safe to assume it'll just be tacked on the end, i.e. after V (and N).

llvm/lib/Target/RISCV/RISCVSystemOperands.td
377
Machine Scalar Cryptography CSRs

? I think these headings come from the ISA manual, but K should be consistent both here and in its spec.

llvm/test/MC/RISCV/rvk/rv32k.s
1 ↗(On Diff #328842)

You should have 3 files:

rv32k-valid.s - Tests the instructions common between RV32K and RV64K, with RUN lines for both

rv32k-only-valid.s - Tests the instructions only present in RV32K

rv64k-valid.s - Tests the additional instructions in RV64K

12 ↗(On Diff #328842)

CHECK lines come before not after

144 ↗(On Diff #328842)

Fix

llvm/test/MC/RISCV/rvk/rv64k.s
126 ↗(On Diff #328842)

Fix

llvm/test/MC/RISCV/rvk/rvk-invalid.s
1 ↗(On Diff #328842)

Needs to be testing both RV32 and RV64 including instructions specific to each, similar to what I said for the tests above.

VincentWu updated this revision to Diff 328925.Mar 7 2021, 7:56 PM
VincentWu marked 10 inline comments as done.

style: formate the whitespace, annotation&code style

VincentWu updated this revision to Diff 328937.Mar 7 2021, 10:14 PM

fix: submit wrong patch

VincentWu updated this revision to Diff 328955.Mar 8 2021, 2:07 AM
VincentWu marked 5 inline comments as done.
VincentWu added inline comments.Mar 10 2021, 6:57 PM
llvm/lib/Target/RISCV/RISCVInstrInfoK.td
10

yes, it is a typo, my fault

46

I'm a little confused about that. So do you mean number rcon is a kind of offset or something?and we need an extra base address?

59

yes, but there will be something wrong whing building if I just remove the Sched<[]>. and we found that the RISCVInstrInfoB.td which is already merged also put an empty Sched<[]> there. So we put it there too.

jrtc27 added inline comments.Mar 11 2021, 4:25 AM
llvm/lib/Target/RISCV/RISCVInstrInfoK.td
19–20

Indenting is still off:

  • 2 spaces not 4
  • we don't typically indent inside lets
46

No. Wikipedia has details (https://en.wikipedia.org/wiki/AES_key_schedule#Round_constants), but basically the the round number counts up 1 by 1, but the actual round constant is a function of the round number (either implemented with a simple table or the slightly more general formula). I've filed https://github.com/riscv/riscv-crypto/issues/82 about this, but given that's what the spec currently calls it I think it's fine to call it rcon in LLVM.

59

That shouldn't be the case if you add HasStdExtK to UnsupportedFeatures for the models

59

Only one space after the colon

67–70

I think it'd be neater to make this a separate "group" (i.e. add a blank line between SHA256SIG1 and SM3P0, and then not align the new group with the sha256 ones), as that'll cut down on the number of spaces

98–105

Line things up properly if you're going to go to the trouble of doing it

llvm/test/MC/RISCV/rvk/rv32k-valid.s
1 ↗(On Diff #328955)

These files probably don't need to be in their own subdirectory. RVV has one, but that's only because it's such a huge extension, whereas this is a much smaller extension so can be like how we're testing all the others.

llvm/test/MC/RISCV/rvk/rvk32-invalid.s
1 ↗(On Diff #328955)

File should be rv32k- not rvk32 (same for the file below)

2 ↗(On Diff #328955)

Need a riscv64 RUN line

VincentWu updated this revision to Diff 330192.Mar 12 2021, 2:59 AM
VincentWu marked 5 inline comments as done.
VincentWu marked an inline comment as done.
VincentWu added inline comments.
llvm/test/MC/RISCV/rvk/rv32k-valid.s
67 ↗(On Diff #328955)

need fix

VincentWu updated this revision to Diff 330443.Mar 13 2021, 4:44 AM
VincentWu marked 4 inline comments as done.
VincentWu updated this revision to Diff 330447.Mar 13 2021, 5:50 AM
  • style: formate the code
  • refactor: use rcon class to limit the imm range
  • Test: add 64bit test case
  • move test cases
  • fix: fix wrong CHECK-INST
VincentWu updated this revision to Diff 330449.Mar 13 2021, 6:19 AM

fix: submit the wrong patch file

asb added inline comments.Mar 18 2021, 3:29 AM
llvm/lib/Target/RISCV/RISCVInstrInfoK.td
10

I'd base this comment on the one in RISCVInstrInfoB.td, better documenting this is a not-yet-ratified extension.

VincentWu updated this revision to Diff 335205.Apr 4 2021, 11:18 PM
VincentWu marked an inline comment as done.

Classifies instructions into their corresponding functional subset
add more test case for Zkg&Zkg which share instructions from B ext

VincentWu added inline comments.Apr 4 2021, 11:21 PM
llvm/lib/Target/RISCV/RISCVSchedRocket.td
19

it seems too long, but we haven't found a good way to shorten them. have any ideas?

Jim added inline comments.Apr 8 2021, 10:05 PM
llvm/lib/Target/RISCV/RISCV.td
190–270

Add blank line

192

and others

222

Alignment

251

Alignment

268

Add blank line

Jim added a reviewer: Jim.Apr 8 2021, 10:06 PM
craig.topper added inline comments.Apr 8 2021, 11:02 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
724

This line has undefined behavior if the user tries to pass a value near INT64_MAX since the +5 would wrap around which isn't undefined.

But I'm not sure it does what you want. This would accept -5, -4, -3, -2, and -1.

llvm/lib/Target/RISCV/RISCVInstrInfoK.td
24

This isUint<4>(Imm) doesn't match what's in the asm parser.

VincentWu marked 6 inline comments as done.
VincentWu added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
724

right it's my fault. I forgot the numbers -5~-1, the spec defines the range of number is [0,10], I limit the upper bound is (1 << 4) -5 but forget it also causes the wrong lower bound. I will fix it soon.

Jim added a comment.Thu, Apr 15, 6:13 AM

Revision Contents look weired. Does this patch upload by git diff -U99999?
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

craig.topper added inline comments.Thu, Apr 15, 9:17 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
724

Space after the first "Imm"

1199

Just write 10. I don't think subtracting 6 from 16 makes this very readability. The other places, the name of the error has the number of bits in its name, so using that bit width(or 1 less than it) in the creation of the constants makes sense.

In D98136#2691388, @Jim wrote:

Revision Contents look weired. Does this patch upload by git diff -U99999?
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

I checked history and made sure I did use the parameter -U99999, so I don't know where the strange part is, I'll upload a new version.

VincentWu updated this revision to Diff 337972.Thu, Apr 15, 8:11 PM
VincentWu marked 2 inline comments as done.
VincentWu updated this revision to Diff 343967.Sun, May 9, 10:59 PM

[RISCV][MC][K-ext] Fix wrong input DAG for ByteSelect cls