This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][CodeGen] Implement IR Intrinsic support for K extension
ClosedPublic

Authored by VincentWu on May 11 2021, 11:42 PM.

Details

Summary

This revision implements IR Intrinsic support for RISCV Scalar Crypto extension according to the specification of version 1.0
Co-author:@ksyx & @VincentWu & @lihongliang & @achieveartificialintelligence

Diff Detail

Event Timeline

ksyx created this revision.May 11 2021, 11:42 PM
ksyx requested review of this revision.May 11 2021, 11:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 11:42 PM
ksyx updated this revision to Diff 344676.EditedMay 11 2021, 11:46 PM

fix: patch does not apply (retrigger ci after adding parent revision)

VincentWu added a subscriber: VincentWu.
Jim added inline comments.May 13 2021, 10:22 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
1254

one space before ":"
And the same case in the following.

ksyx updated this revision to Diff 345366.May 14 2021, 1:10 AM
ksyx marked an inline comment as done.

refactor: remove redundant spaces around colons

ksyx updated this revision to Diff 346984.May 21 2021, 4:21 AM

refactor: fix spaces

ksyx updated this revision to Diff 347346.May 24 2021, 4:31 AM

fix: types of immargs to better fit the proposal

liaolucy commandeered this revision.Oct 10 2021, 6:23 PM
liaolucy added a reviewer: ksyx.
craig.topper added inline comments.Jan 24 2022, 7:53 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
259 ↗(On Diff #381463)

This should be done in the else block where hasStdExtZbb() is handled. Zbp is for GREVI.

2427 ↗(On Diff #381463)

This should not be needed with the other change.

llvm/lib/Target/RISCV/RISCVInstrInfoZk.td
24 ↗(On Diff #381463)

This needs to be rebased on the current Zk patch.

211 ↗(On Diff #381463)

There are no _K instructions in the most recent Zk patches.

All Zb* related changes should be in RISCVInstrInfoZb.td

VincentWu commandeered this revision.Jan 24 2022, 6:42 PM
VincentWu added a reviewer: liaolucy.
VincentWu updated this revision to Diff 402834.Jan 25 2022, 3:41 AM
VincentWu marked 4 inline comments as done.

rebase and update

VincentWu updated this revision to Diff 402835.Jan 25 2022, 3:51 AM

rename testcase

VincentWu edited the summary of this revision. (Show Details)Jan 25 2022, 3:53 AM
VincentWu updated this revision to Diff 402849.Jan 25 2022, 4:37 AM

fix intrinsic

VincentWu updated this revision to Diff 402875.Jan 25 2022, 5:49 AM

fix intrinsic and testcase

craig.topper added inline comments.Jan 25 2022, 10:51 AM
llvm/include/llvm/IR/IntrinsicsRISCV.td
116

Capital Z

117

rev8 is the same as llvm.bswap.i32 or llvm.bswap.i64. We don't need an intrinsic for it.

1373

Capitalize Z in the comments.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
282 ↗(On Diff #402875)

You want Zbkb in addition to Zbb here.

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
892 ↗(On Diff #402875)

This should stay Zbp only.

901 ↗(On Diff #402875)

This should stay Zbp only.

1196 ↗(On Diff #402875)

We don't need int_riscv_rev8. We can use bswap

llvm/test/CodeGen/RISCV/rv32zbc-zbkc-intrinsic.ll
5 ↗(On Diff #402875)

Can we use a single check-prefix?

llvm/test/CodeGen/RISCV/rv32zbp-zbkb.ll
7 ↗(On Diff #402875)

Can we use one check-prefix for Zbp and Zbkb?

llvm/test/CodeGen/RISCV/rv64zbb-zbp-zbkb.ll
9 ↗(On Diff #402875)

Can we use one check-prefix for Zbb, Zbp, and Zbkb?

llvm/test/CodeGen/RISCV/rv64zbc-zbkc-intrinsic.ll
5 ↗(On Diff #402875)

Can we use a single check-prefix?

llvm/test/CodeGen/RISCV/rv64zbp-zbkb.ll
7 ↗(On Diff #402875)

Can we use one check-prefix for Zbp and Zbkb?

llvm/test/CodeGen/RISCV/rv64zksed-intrinsic.ll
13 ↗(On Diff #402875)

Something went wrong here

29 ↗(On Diff #402875)

And here

VincentWu updated this revision to Diff 403134.Jan 25 2022, 9:59 PM
VincentWu marked 14 inline comments as done.

address comments

craig.topper added inline comments.Jan 25 2022, 10:09 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
282 ↗(On Diff #402875)

Does this line exceed 80 columns?

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
890 ↗(On Diff #403134)

Revert this

892 ↗(On Diff #403134)

Revert this

899 ↗(On Diff #403134)

Revert this

remove duplicate code & clang-format

VincentWu marked 4 inline comments as done.Jan 25 2022, 10:24 PM
craig.topper added inline comments.Jan 26 2022, 1:40 PM
llvm/lib/Target/RISCV/RISCV.td
175 ↗(On Diff #403139)

The string for Zbb was recently updated. Please update to match.

llvm/test/CodeGen/RISCV/rv32zbb-zbp-zbkb.ll
9 ↗(On Diff #403139)

Can we share a prefix for Zbb/Zbp/Zbkb?

llvm/test/CodeGen/RISCV/rv64zbb-zbp-zbkb.ll
507 ↗(On Diff #403139)

This a bug that Zbp doesn't match Zbb. I'll fix it. You'll need to add Zbkb to RISCVTargetLowering::hasAndNotCompare in this patch.

VincentWu updated this revision to Diff 403480.Jan 26 2022, 8:01 PM
VincentWu marked 3 inline comments as done.

update & address comment

This revision is now accepted and ready to land.Jan 26 2022, 9:47 PM

rebase & read to submit

This revision was landed with ongoing or failed builds.Jan 26 2022, 11:53 PM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.Feb 13 2022, 7:17 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
1372

Why does this intrinsic have the Returned attribute?

ksyx added inline comments.Feb 13 2022, 7:27 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
1372

Maybe this is following the old 0.9.0 version that mixes the use of rs1/rd registers.

craig.topper added inline comments.Feb 13 2022, 7:35 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
1372

I see, but that was still a mistake. The Returned property means that the *value* passed into the argument is the *value* that is returned. Even if rd/rs1 are the same register, the operation still changes the value in that register.

ksyx added inline comments.Feb 13 2022, 7:38 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
1372

Thanks for pointing out this as I may misunderstood this while implementing.

llvm/include/llvm/IR/IntrinsicsRISCV.td
1372

Thanks! I've committed in 352e19c.