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

[VincentWu]

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

[VincentWu]

fix: fix error trailing whitespace when building

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoK.td

10

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

[VincentWu]

typo: RISC-K ->RISC-V

[jrtc27]

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

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

33

Blank line before

40

Blank line after

llvm/lib/Target/RISCV/RISCV.td

11

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

117

Should be fixed

llvm/lib/Target/RISCV/RISCVSubtarget.h

3

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

4

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]

style: formate the whitespace, annotation&code style

[VincentWu]

fix: submit wrong patch

[VincentWu]

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]

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]

llvm/test/MC/RISCV/rvk/rv32k-valid.s

67 ↗

(On Diff #328955)

need fix

[VincentWu]

• 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]

fix: submit the wrong patch file

[asb]

llvm/lib/Target/RISCV/RISCVInstrInfoK.td

9

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

[VincentWu]

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

[VincentWu]

llvm/lib/Target/RISCV/RISCVSchedRocket.td

0–1

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

[Jim]

llvm/lib/Target/RISCV/RISCV.td

5–1

Add blank line

7

and others

37

Alignment

66

Alignment

83

Add blank line

[craig.topper]

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

39

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]

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

39

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]

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]

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

34

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.

39

Space after the first "Imm"

[VincentWu]

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]

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

[kito-cheng]

Could you handle k* extension for ELF attribute?

RISCVAsmParser::parseDirectiveAttribute@lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
RISCVTargetStreamer::emitTargetAttributes@MCTargetDesc/RISCVTargetStreamer.cpp

[VincentWu]

handle k* extension for ELF attribute