This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add combination crypto extensions in ISAInfo
ClosedPublic

Authored by eopXD on Feb 11 2022, 1:49 AM.

Details

Summary

The crypto extension have several shorthand extensions that don't consist of any extra instructions.
Take zk for example, while the extension would imply zkn, zkr, zkt. The 3 extensions should also
combine back into zk to maintain the canonical order in isa strings.

This patch addresses the above.

Diff Detail

Unit TestsFailed

Event Timeline

eopXD created this revision.Feb 11 2022, 1:49 AM
eopXD requested review of this revision.Feb 11 2022, 1:49 AM
eopXD retitled this revision from [RISCV] Add combination for ISAInfo - zkn, zkr, zkt -> zk to [RISCV] Add combination crypto extensions in ISAInfo.Feb 11 2022, 1:55 AM
eopXD edited the summary of this revision. (Show Details)
eopXD retitled this revision from [RISCV] Add combination crypto extensions in ISAInfo to [WIP][RISCV] Add combination crypto extensions in ISAInfo.EditedFeb 11 2022, 2:02 AM

I will also add zkn, zks later.

eopXD updated this revision to Diff 407832.Feb 11 2022, 3:42 AM
  • Add zkn, zks
  • Follow clang-format
eopXD retitled this revision from [WIP][RISCV] Add combination crypto extensions in ISAInfo to [RISCV] Add combination crypto extensions in ISAInfo.Feb 11 2022, 3:44 AM
VincentWu added inline comments.Feb 12 2022, 10:54 PM
llvm/lib/Support/RISCVISAInfo.cpp
760

i'm not sure did you notice that?

could you explain what's different about this patch?

craig.topper added inline comments.Feb 12 2022, 11:46 PM
llvm/lib/Support/RISCVISAInfo.cpp
760

I believe this patch is inferring Zk when all of zkn, zkr, and zkt are listed in march. This is needed so that __riscv_zk gets defined in the frontend preprocessor when the individual extensions are listed individually.

VincentWu added inline comments.Feb 13 2022, 1:45 AM
llvm/lib/Support/RISCVISAInfo.cpp
760

oh, yes. you are right. Then could we just use one variable if the value of ImpliedExtsZk and CombineIntoZk are entirely same?

cuz I think it is better to maintain if we combine them.

850

could we use ImpliedExtsZk here if they have same value?

eopXD updated this revision to Diff 408252.Feb 13 2022, 5:01 AM
eopXD marked 4 inline comments as done.

Address comment, reuse existing strings.

VincentWu added inline comments.Feb 16 2022, 7:27 PM
llvm/test/CodeGen/RISCV/attributes.ll
146

I know that the FeatureBits has include the Zk after updateCombination. but this test case doesn't check it.

what about test that in clang/test/Preprocessor/riscv-target-features.c

VincentWu added inline comments.Feb 16 2022, 7:31 PM
llvm/test/CodeGen/RISCV/attributes.ll
146

what about test that in clang/test/Preprocessor/riscv-target-features.c

ok, this patch is for LLVM, so ignore it.

but this problem is still here,“this test case doesn't check it.”

eopXD added inline comments.Feb 21 2022, 7:14 PM
llvm/test/CodeGen/RISCV/attributes.ll
146

The test case provides zkn, zkr, zkt and expects to have zk1p0 within the generated attributes. I think this will cover the case added by updateCombination?

; RUN: llc -mtriple=riscv64 -mattr=+zkn,+zkr,+zkt %s -o - | FileCheck --check-prefix=RV64COMBINEINTOZK %s

Maybe I had misread your concern, may you rephrase if so?

kito-cheng added inline comments.Feb 23 2022, 10:09 PM
llvm/test/CodeGen/RISCV/attributes.ll
146

Adding test case to clang/test/Preprocessor/riscv-target-features.c to make sure __riscv_zk has defined properly for -march=rv64i_zkn_zkr_zkt/-march=rv32i_zkn_zkr_zkt?

eopXD updated this revision to Diff 411014.Feb 23 2022, 11:27 PM

Add test for clang.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2022, 11:27 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eopXD marked an inline comment as done.Feb 23 2022, 11:28 PM

Friendly ping @VincentWu ;)
Any comments to the latest response?

Gentle ping

This revision is now accepted and ready to land.Feb 28 2022, 10:33 PM
eopXD updated this revision to Diff 413688.Mar 7 2022, 7:45 PM

Rebase to latest main

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 7:45 PM
This revision was landed with ongoing or failed builds.Mar 8 2022, 9:52 AM
This revision was automatically updated to reflect the committed changes.