This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Adjust predicates and update intrinsic for clmul and clmulh in Zbkc extension
AbandonedPublic

Authored by Jimerlife on Jan 23 2022, 6:11 PM.

Details

Summary

Adjust predicates and update intrinsic for clmul and clmulh in Zbkc extension

Diff Detail

Event Timeline

Jimerlife created this revision.Jan 23 2022, 6:11 PM
Jimerlife requested review of this revision.Jan 23 2022, 6:11 PM

Actually this patch is NOT NFC, you fixed an issue that is: zbkc isn't include cmul and clmulh.

So I would suggest you:

  1. Remove NFC from the title.
  2. Add test case to test cmul and clmulh is available for zbkc after this patch.

Will you also be updating clang's RISCVBuiltins.def?

Can you add tests?

craig.topper requested changes to this revision.Jan 23 2022, 6:36 PM
This revision now requires changes to proceed.Jan 23 2022, 6:36 PM

Do you add intrinsic?

we add init support https://reviews.llvm.org/D102310

Jimerlife retitled this revision from [RISCV][NFC] Add "zbkc" predicate for clmul and clmulh pattern to [RISCV] Adjust predicates and update intrinsic for clmul and clmulh in Zbkc extension .
Jimerlife edited the summary of this revision. (Show Details)
Jimerlife added a reviewer: kito-cheng.
  1. Add IR tests for clmul and clmulh in zbkc extension.
  2. Updating BuiltinsRISCV.def, I rename "builtinriscvclmul" to "builtin_riscv_clmul_kc" in zbkc extension, because cannot share same name "builtinriscv_clmul" in zbc and zbkc extension.
  3. Add C tests for clmul and clmulh in zbkc extension
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2022, 12:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Will you also be updating clang's RISCVBuiltins.def?

Can you add tests?

I update BuiltinsRISCV.def and add "__builtin_riscv_clmul_kc" intrinsic for zbkc extension, but I am not sure if the name is appropriate.

Actually this patch is NOT NFC, you fixed an issue that is: zbkc isn't include cmul and clmulh.

So I would suggest you:

  1. Remove NFC from the title.
  2. Add test case to test cmul and clmulh is available for zbkc after this patch.

Thank you. I adjust patch according your suggestions.

craig.topper added inline comments.Jan 24 2022, 1:03 AM
llvm/include/llvm/IR/IntrinsicsRISCV.td
91

Add spaces around “or”

Jimerlife updated this revision to Diff 402426.Jan 24 2022, 1:23 AM

add spaces around "or"

Jimerlife marked an inline comment as done.Jan 24 2022, 1:23 AM

clang Zbkc patch: https://reviews.llvm.org/D112774, If there are any mistakes, you can help to point them out

Will you also be updating clang's RISCVBuiltins.def?

Can you add tests?

I update BuiltinsRISCV.def and add "__builtin_riscv_clmul_kc" intrinsic for zbkc extension, but I am not sure if the name is appropriate.

https://github.com/rvkrypto/rvkrypto-fips/blob/main/rvkintrin.md
Is the name as __builtin_riscv_clmul_32 or __builtin_riscv_clmul_64 better for conflict resolution?

VincentWu added a comment.EditedJan 24 2022, 7:43 AM

Hi, Jimerlife

it is a great work to implement the intrinsic of zbkb.
however, I don't know whether you have noticed that there is already to patch https://reviews.llvm.org/D112774 and https://reviews.llvm.org/D102310.
these two patches have included almost all the work you did in your patch.

I think we should better avoid overlap as much as possible.

therefore, could you please comment on the difference between these two patches as you see it?
let's focus on only one version.

I strongly suggest you to join the bi-weekly LLVM RISC-V meetings to discuss with us if possible. Almost all contributors on RISC-V backend would attend.

Will you also be updating clang's RISCVBuiltins.def?

Can you add tests?

I update BuiltinsRISCV.def and add "__builtin_riscv_clmul_kc" intrinsic for zbkc extension, but I am not sure if the name is appropriate.

https://github.com/rvkrypto/rvkrypto-fips/blob/main/rvkintrin.md
Is the name as __builtin_riscv_clmul_32 or __builtin_riscv_clmul_64 better for conflict resolution?

The builtins for clmul have been in for months without _32 or _64 since they operate on the "long" data type.

craig.topper added inline comments.Jan 24 2022, 9:15 AM
clang/include/clang/Basic/BuiltinsRISCV.def
28

After D117854, it should be possible to use

TARGET_BUILTIN(__builtin_riscv_clmul, "LiLiLi", "nc", "zbc|zbkc")
TARGET_BUILTIN(__builtin_riscv_clmulh, "LiLiLi", "nc", "zbc|zbkc")
Jimerlife abandoned this revision.Jan 24 2022, 5:49 PM