Details

Reviewers

kito-cheng
MaskRay
craig.topper
HsiangKai
asb
jrtc27
Jim

Commits

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

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

Repository: rG LLVM Github Monorepo

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

Harbormaster completed remote builds in B129486: Diff 380603.Oct 19 2021, 1:55 AM

achieveartificialintelligence added a child revision: D112774: [RISCV] Support k-ext clang intrinsics.Oct 31 2021, 6:11 PM

Ping

Herald added a subscriber: luke957. · View Herald TranscriptNov 14 2021, 4:54 PM

Rebase and Ping.

Harbormaster completed remote builds in B135328: Diff 388783.Nov 21 2021, 6:54 PM

liaolucy added a subscriber: liaolucy.Dec 9 2021, 1:14 AM

liaolucy added inline comments.

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
83	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?

alextsao1999 added a subscriber: alextsao1999.Dec 11 2021, 1:17 PM

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

Harbormaster completed remote builds in B141440: Diff 397212.Jan 4 2022, 12:23 AM

VincentWu updated this revision to Diff 397218.Jan 4 2022, 12:32 AM

Harbormaster completed remote builds in B141446: Diff 397218.Jan 4 2022, 1:57 AM

liaolucy added inline comments.Jan 4 2022, 5:08 AM

llvm/lib/Support/RISCVISAInfo.cpp
99	Should zbkb extension be removed?

liaolucy added inline comments.Jan 4 2022, 5:19 AM

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
11	Is version 1.0-rc4? The latest version is 1.0.0-rc6.

fix issues mentioned on comment

Harbormaster completed remote builds in B141601: Diff 397446.Jan 4 2022, 8:29 PM

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

Herald added a subscriber: eopXD. · View Herald TranscriptJan 17 2022, 12:24 AM

craig.topper added inline comments.Jan 17 2022, 12:34 AM

llvm/lib/Support/RISCVISAInfo.cpp
54	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
54	line this up with funct3 on the previous line
62	Indent ths two more spaces
70	indent this 2 more spaces to line up with funct3
llvm/lib/Target/RISCV/RISCVSystemOperands.td
363	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
108	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
202	Is this Predicate needed? Zkn is just a convenience to enable multiple extensions. No instructions belong to it right?
211	Is this Predicate needed? Zks is just a convenience to enable multiple extensions. No instructions belong to it right?
218	This Predicate doesn't appear to be used on any instructions. Just the scheduler model
228	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
38	Is this class used?
45	Is this class used?
54	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.
72	This line is over indented

fix comment

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

add new line at end of file

Harbormaster completed remote builds in B143959: Diff 400758.Jan 18 2022, 1:30 AM

emove ext verision number

Harbormaster completed remote builds in B144010: Diff 400832.Jan 18 2022, 7:34 AM

craig.topper added inline comments.Jan 20 2022, 11:01 AM

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1254	`(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
2	K -> Zk in both places
10	K -> Zk
25	This ImmLeaf should check 0-10. Alternatively it can be removed until CodeGen support is added.
29	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–23	This line needs to rebased I think. I'll also be removed Zvlsseg later today.

rebase and fix comments

Harbormaster completed remote builds in B144753: Diff 401878.Jan 21 2022, 1:24 AM

updete

Harbormaster completed remote builds in B144784: Diff 401912.Jan 21 2022, 3:37 AM

rebase and update

Harbormaster completed remote builds in B144973: Diff 402163.Jan 21 2022, 8:27 PM

craig.topper added inline comments.Jan 23 2022, 12:37 PM

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1150	Missing "break;" This will be a problem the next time someone adds a case after this one.
llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
2	Scala -> Scalar
10	ZK -> Zk

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

address comment & remove [RFC] from title

LGTM

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

rebase

Herald added a subscriber: • pcwang-thead. · View Herald TranscriptJan 23 2022, 8:01 PM

In D98136#3265096, @craig.topper wrote:

LGTM

Thanks -)

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1150	oops, sorry my bad, fixed

Harbormaster completed remote builds in B145153: Diff 402389.Jan 23 2022, 10:39 PM

Closed by commit rGe29d8fb16978: [RISCV] Initially support the K-extension instructions on the LLVM MC layer (authored by VincentWu). · Explain WhyJan 23 2022, 10:45 PM

This revision was automatically updated to reflect the committed changes.

VincentWu added a commit: rGe29d8fb16978: [RISCV] Initially support the K-extension instructions on the LLVM MC layer.

VincentWu mentioned this in D117889: [RISCV] add support for zbkx subextension in MC layer..Jan 23 2022, 10:54 PM

RKSimon added a subscriber: RKSimon.Jan 26 2022, 9:58 AM

RKSimon added inline comments.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1149	@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	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?

VincentWu mentioned this in D118323: [RISCV] fix dead code.Jan 26 2022, 11:51 PM

VincentWu mentioned this in rG6a4d3f37b5a7: [RISCV] fix dead code.Jan 27 2022, 12:00 AM

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 402402

llvm/lib/Support/RISCVISAInfo.cpp

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

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

llvm/lib/Target/RISCV/RISCV.td

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

llvm/lib/Target/RISCV/RISCVInstrInfo.td

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td

llvm/lib/Target/RISCV/RISCVSchedRocket.td

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td

llvm/lib/Target/RISCV/RISCVSubtarget.h

llvm/lib/Target/RISCV/RISCVSystemOperands.td

llvm/test/CodeGen/RISCV/attributes.ll

llvm/test/MC/RISCV/attribute-arch.s

llvm/test/MC/RISCV/rv32zknd-only-invalid.s

llvm/test/MC/RISCV/rv32zknd-only-valid.s

llvm/test/MC/RISCV/rv32zkne-only-invalid.s

llvm/test/MC/RISCV/rv32zkne-only-valid.s

llvm/test/MC/RISCV/rv32zknh-only-valid.s

llvm/test/MC/RISCV/rv32zknh-valid.s

llvm/test/MC/RISCV/rv32zksed-invalid.s

llvm/test/MC/RISCV/rv32zksed-valid.s

llvm/test/MC/RISCV/rv32zksh-valid.s

llvm/test/MC/RISCV/rv64zknd-only-valid.s

llvm/test/MC/RISCV/rv64zkne-only-invalid.s

llvm/test/MC/RISCV/rv64zkne-only-valid.s

llvm/test/MC/RISCV/rv64zknh-only-valid.s

llvm/test/MC/RISCV/rv64zksed-invalid.s

llvm/test/MC/RISCV/rvk-user-csr-name.s

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 402402

llvm/lib/Support/RISCVISAInfo.cpp

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

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

llvm/lib/Target/RISCV/RISCV.td

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

llvm/lib/Target/RISCV/RISCVInstrInfo.td

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td

llvm/lib/Target/RISCV/RISCVSchedRocket.td

llvm/lib/Target/RISCV/RISCVSchedSiFive7.td

llvm/lib/Target/RISCV/RISCVSubtarget.h

llvm/lib/Target/RISCV/RISCVSystemOperands.td

llvm/test/CodeGen/RISCV/attributes.ll

llvm/test/MC/RISCV/attribute-arch.s

llvm/test/MC/RISCV/rv32zknd-only-invalid.s

llvm/test/MC/RISCV/rv32zknd-only-valid.s

llvm/test/MC/RISCV/rv32zkne-only-invalid.s

llvm/test/MC/RISCV/rv32zkne-only-valid.s

llvm/test/MC/RISCV/rv32zknh-only-valid.s

llvm/test/MC/RISCV/rv32zknh-valid.s

llvm/test/MC/RISCV/rv32zksed-invalid.s

llvm/test/MC/RISCV/rv32zksed-valid.s

llvm/test/MC/RISCV/rv32zksh-valid.s

llvm/test/MC/RISCV/rv64zknd-only-valid.s

llvm/test/MC/RISCV/rv64zkne-only-invalid.s

llvm/test/MC/RISCV/rv64zkne-only-valid.s

llvm/test/MC/RISCV/rv64zknh-only-valid.s

llvm/test/MC/RISCV/rv64zksed-invalid.s

llvm/test/MC/RISCV/rvk-user-csr-name.s

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

Revision Contents
Changeset List