This is an archive of the discontinued LLVM Phabricator instance.

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

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 v1.0.0 of K Ext specification(scala crypto has been ratified already). 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 directory.

Remove the subfeature of Zbk* which conflict with b extensions to reduce the size of the patch.
(Zbk* will be resubmit after this patch has been merged)

Co-author:@ksyx & @VincentWu & @lihongliang & @achieveartificialintelligence

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jrtc27 added inline comments.Mar 7 2021, 8:00 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
23

Blank line before

30

Blank line after

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

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

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

103

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 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
2

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
2

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

3

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

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
0–1

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
5–1

Add blank line

7

and others

37

Alignment

66

Alignment

83

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
29

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
29

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.Apr 15 2021, 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.Apr 15 2021, 9:17 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
24

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.

29

Space after the first "Imm"

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.Apr 15 2021, 8:11 PM
VincentWu marked 2 inline comments as done.
VincentWu updated this revision to Diff 343967.May 9 2021, 10:59 PM

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

Could you handle k* extension for ELF attribute?

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

handle k* extension for ELF attribute

VincentWu updated this revision to Diff 372067.Sep 11 2021, 2:37 AM
VincentWu edited the summary of this revision. (Show Details)

update the implemention to adopt the spec v1.0.0-rc1
co-worker @lihongliang

VincentWu edited the summary of this revision. (Show Details)Sep 11 2021, 2:39 AM
VincentWu updated this revision to Diff 372068.Sep 11 2021, 3:19 AM
  • [RISCV] style: formate the code of RISCVAsmParser.cpp
This comment was removed by VincentWu.
VincentWu edited the summary of this revision. (Show Details)Oct 18 2021, 10:42 AM
VincentWu updated this revision to Diff 380603.Oct 19 2021, 1:14 AM
VincentWu edited the summary of this revision. (Show Details)
VincentWu added a comment.EditedOct 19 2021, 1:14 AM

update the code to adopt the spec v1.0.0-rc4
co-worker @achieveartificialintelligence

VincentWu marked an inline comment as done.Oct 19 2021, 1:27 AM
VincentWu edited the summary of this revision. (Show Details)Oct 19 2021, 1:29 AM
liaolucy added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
82 ↗(On Diff #388783)

ROR_K instruction is the same as the B-ext.
./RISCVInstrInfoZb.td: def ROR : ALU_rr<0b0110000, 0b101, "ror">,

Do we need to redefine this instruction? Does anyone have any Suggestions?

VincentWu updated this revision to Diff 397212.EditedJan 4 2022, 12:22 AM
VincentWu added a subscriber: nannan1025.

add Entropy Source CSR

Remove the subfeature of Zbk* which conflict with b extensions to reduce the size of the patch.
(Zbk* will be resubmit after this patch has been merged)

Co-author:@nannan1025

VincentWu updated this revision to Diff 397218.Jan 4 2022, 12:32 AM
liaolucy added inline comments.Jan 4 2022, 5:08 AM
llvm/lib/Support/RISCVISAInfo.cpp
64 ↗(On Diff #397218)

Should zbkb extension be removed?

liaolucy added inline comments.Jan 4 2022, 5:19 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
10 ↗(On Diff #397218)

Is version 1.0-rc4? The latest version is 1.0.0-rc6.

VincentWu updated this revision to Diff 397446.Jan 4 2022, 7:20 PM
VincentWu marked 2 inline comments as done.

fix issues mentioned on comment

VincentWu edited the summary of this revision. (Show Details)Jan 4 2022, 10:24 PM
VincentWu edited the summary of this revision. (Show Details)Jan 6 2022, 7:50 AM

PING

The latest spec (rc-6) of the scalar crypto extension has been released for almost two months. That means that the scalar crypto extensions have become stable.
Also the GCC seems to have merged the patch for scalar crypto extensions into the upstream
So I think it is time to decide whether this patch should be merged or not.

Although I understand that this patch may be a bit large, I would still like someone to review it again and tell me if this patch can be merged or still needs to be modified

craig.topper added inline comments.Jan 17 2022, 12:34 AM
llvm/lib/Support/RISCVISAInfo.cpp
53 ↗(On Diff #397446)

This needs to be rebased. Zba/b/c/s were all moved to non-experimental recently

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
430 ↗(On Diff #397446)

This looks unrelated to the rest of the patch.

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
53 ↗(On Diff #397446)

line this up with funct3 on the previous line

61 ↗(On Diff #397446)

Indent ths two more spaces

69 ↗(On Diff #397446)

indent this 2 more spaces to line up with funct3

llvm/lib/Target/RISCV/RISCVSystemOperands.td
380

Space after the comma like the rest of the file

eopXD added inline comments.Jan 17 2022, 12:36 AM
llvm/lib/Support/RISCVISAInfo.cpp
73 ↗(On Diff #397446)

Regarding zkn, zks, zk, D112359 added extension implication as a separate function. I think you may need to rebase and do some modifications.

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
430 ↗(On Diff #397446)

Cleanup?

craig.topper added inline comments.Jan 17 2022, 10:30 AM
llvm/lib/Target/RISCV/RISCV.td
194

Is this Predicate needed? Zkn is just a convenience to enable multiple extensions. No instructions belong to it right?

203

Is this Predicate needed? Zks is just a convenience to enable multiple extensions. No instructions belong to it right?

210

This Predicate doesn't appear to be used on any instructions. Just the scheduler model

220

Is this Predicate needed? Zk is just a convenience to enable multiple extensions. No instructions belong to it right?

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
37 ↗(On Diff #397446)

Is this class used?

44 ↗(On Diff #397446)

Is this class used?

53 ↗(On Diff #397446)

I don't think we should put Sched<[]> inside the generic classes. We'll need to be able to add per instruction information in the future.

71 ↗(On Diff #397446)

This line is over indented

VincentWu marked 16 inline comments as done.
VincentWu edited the summary of this revision. (Show Details)

fix comment

scala crypto has been ratified already, remove description "-rc6"

add new line at end of file

VincentWu updated this revision to Diff 400832.Jan 18 2022, 7:02 AM

emove ext verision number

craig.topper added inline comments.Jan 20 2022, 11:01 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1191

(1 << 4) - 6 -> 10. There's no reason to write a more complex expression. It makes more sense for things like Match_InvalidUImm2 to show the connection between the name of the error and the integer values. That doesn't apply here.

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
1 ↗(On Diff #400832)

K -> Zk in both places

9 ↗(On Diff #400832)

K -> Zk

24 ↗(On Diff #400832)

This ImmLeaf should check 0-10. Alternatively it can be removed until CodeGen support is added.

28 ↗(On Diff #400832)

It's not a UIMM4. It should have its own OPERAND type and the range check for it in X86InstrInfo.cpp should check 0-10.

llvm/lib/Target/RISCV/RISCVSchedRocket.td
19

This line needs to rebased I think. I'll also be removed Zvlsseg later today.

VincentWu marked 6 inline comments as done.

rebase and fix comments

VincentWu updated this revision to Diff 402163.Jan 21 2022, 7:46 PM

rebase and update

craig.topper added inline comments.Jan 23 2022, 12:37 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1150 ↗(On Diff #402163)

Missing "break;" This will be a problem the next time someone adds a case after this one.

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
1 ↗(On Diff #402163)

Scala -> Scalar

9 ↗(On Diff #402163)

ZK -> Zk

Please remove [RFC] from the review name. We shouldn't have that in the commit message.

VincentWu updated this revision to Diff 402382.Jan 23 2022, 6:55 PM
VincentWu marked 3 inline comments as done.
VincentWu retitled this revision from [RISCV][RFC] Initially support the K-extension instructions on the LLVM MC layer to [RISCV] Initially support the K-extension instructions on the LLVM MC layer.

address comment & remove [RFC] from title

This revision is now accepted and ready to land.Jan 23 2022, 7:07 PM

LGTM

Thanks -)

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1150 ↗(On Diff #402163)

oops, sorry my bad, fixed

RKSimon added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1149 ↗(On Diff #402402)

@VincentWu Coverity is warning that this is dead code as RISCVOp::OPERAND_RVKRNU > RISCVOp::OPERAND_LAST_RISCV_IMM

VincentWu added inline comments.Jan 26 2022, 11:32 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1149 ↗(On Diff #402402)

I am very sorry for this error, how do I fix it?
i mean, should I create a new Differential Revision? Or modify the commit in some way?