Page MenuHomePhabricator

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

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
62

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

71

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

78

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

88

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
24

(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
20 ↗(On Diff #400832)

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?