On Tue, Dec 5, 2023 at 1:05 PM Liao Shihua <shi...@iscas.ac.cn> wrote: > > > It's a little patch add just provides a mapping from the RV intrinsic to the > builtin > names within GCC.
Thanks for working on this! I checked with ./contrib/check_GNU_style, which found a two issues: * Trailing whitespace (most likely caused by the CRLF line terminators) * There should be exactly one space between function name and parenthesis. Also, I don't like so much the fact that the comments in the "#endif" lines don't match the "#if". E.g., we have "#if defined(__riscv_zksed)" and "#endif // ZKSED". I would expect that, in this case, we have "#endif // __riscv_zksed". In my comments for v1 I asked about the reasons for using macros vs inline-functions. Craig Topper answered this by clarifying that immediate won't be propagated in case of -O0, because inlining will be disabled (can be tested via __OPTIMIZE__). He also linked to existing code that shows how this is handled in existing code: https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/xmmintrin.h#L52 We should follow this pattern for intrinsics that have immediate arguments (others are fine): #ifdef __OPTIMIZE__ // inline function #else // macro #endif Regarding the tests, I think the dg-skip-if for "-g" and "-flto" is fine. But the dg-options lines seem to be too restrictive (e.g. "-O0" is skipped). The following should be enough for RV64 tests: /* { dg-options "-march=rv64gc_zknd_zkne_zknh_zksed_zksh -mabi=lp64" } */ Nit: I don't mind if we have a "-32" and a "-64" test file, but I would rename them accordingly ("-1.c" -> "-32.c" and "-2.c" -> "-64.c"). Alternatively, if you prefer to merge both files into one, this can be done using the following dg-options in one file: /* { dg-options "-march=rv64gc_zknd_zkne_zknh_zksed_zksh" { target { rv64 } } } */ /* { dg-options "-march=rv32gc_zknd_zkne_zknh_zksed_zksh" { target { rv32 } } } */ The scan-assembler lines can then be adjusted with the "{ target { rvNN } }" as well: /* { dg-final { scan-assembler-times "aes64ds\t" 1 { target { rv64 } } } } */ And the tests can be separated for RV32/RV64 using #if __riscv_xlen == 64 // tests for RV64 only #else // tests for RV32 only #endif // __riscv_xlen == 64 BR Christoph > > Liao Shihua (2): > Add C intrinsics of Scalar Crypto Extension > Add C intrinsics of Bitmanip Extension > > gcc/config.gcc | 2 +- > gcc/config/riscv/riscv-builtins.cc | 22 ++ > gcc/config/riscv/riscv-types.def | 2 + > gcc/config/riscv/riscv-scalar-crypto.def | 18 ++ > gcc/config/riscv/riscv_bitmanip.h | 297 ++++++++++++++++++ > gcc/config/riscv/riscv_crypto.h | 280 +++++++++++++++++ > .../riscv/scalar_bitmanip_intrinsic-1.c | 97 ++++++ > .../riscv/scalar_bitmanip_intrinsic-2.c | 115 +++++++ > .../riscv/scalar_crypto_intrinsic-1.c | 115 +++++++ > .../riscv/scalar_crypto_intrinsic-2.c | 122 +++++++ > 10 files changed, 1069 insertions(+), 1 deletion(-) > create mode 100644 gcc/config/riscv/riscv_bitmanip.h > create mode 100644 gcc/config/riscv/riscv_crypto.h > create mode 100644 > gcc/testsuite/gcc.target/riscv/scalar_bitmanip_intrinsic-1.c > create mode 100644 > gcc/testsuite/gcc.target/riscv/scalar_bitmanip_intrinsic-2.c > create mode 100644 gcc/testsuite/gcc.target/riscv/scalar_crypto_intrinsic-1.c > create mode 100644 gcc/testsuite/gcc.target/riscv/scalar_crypto_intrinsic-2.c > > -- > 2.34.1 >
