Re: [PATCH V2 0/2] RISC-V: Add intrinsics for Bitmanip and Scalar Crypto extensions

2023-12-07 Thread Jeff Law




On 12/7/23 09:59, Christoph Müllner wrote:

On Thu, Dec 7, 2023 at 11:18 AM Liao Shihua  wrote:


In accordance with the suggestions of Christoph Müllner, the following 
amendments are made

Update v1 -> v2:
   1. Rename *_intrinsic-* to *_intrinsic-XLEN.
   2. Typo fix.
   3. Intrinsics with immediate arguments will use marcos at O0 .

It's a little patch add just provides a mapping from the RV intrinsics to the 
builtin
names within GCC.


Thanks for the update!

I think this patchset was not properly tested as I see the tests failing.

Thanks for pointing this out.

I think we're well past the point where if a patchkit doesn't specify 
how it was tested that it should be rejected.  This is standard 
procedure for the rest of the compiler and there's no good reason why we 
should have a lax policy in the RISC-V target files.


Jeff


Re: [PATCH V2 0/2] RISC-V: Add intrinsics for Bitmanip and Scalar Crypto extensions

2023-12-07 Thread Christoph Müllner
On Thu, Dec 7, 2023 at 11:18 AM Liao Shihua  wrote:
>
> In accordance with the suggestions of Christoph Müllner, the following 
> amendments are made
>
> Update v1 -> v2:
>   1. Rename *_intrinsic-* to *_intrinsic-XLEN.
>   2. Typo fix.
>   3. Intrinsics with immediate arguments will use marcos at O0 .
>
> It's a little patch add just provides a mapping from the RV intrinsics to the 
> builtin
> names within GCC.

Thanks for the update!

I think this patchset was not properly tested as I see the tests failing.

$ /opt/riscv-mainline/bin/riscv64-unknown-linux-gnu-gcc
-march=rv64gc_zbb_zbc_zbkb_zbkc_zbkx -mabi=lp64d
/home/cm/src/gcc/riscv-mainline/gcc/testsuite/gcc.target/riscv/scalar_bitmanip_intrinsic-64.c
In file included from
/home/cm/src/gcc/riscv-mainline/gcc/testsuite/gcc.target/riscv/scalar_bitmanip_intrinsic-64.c:5:
/opt/riscv-mainline/lib/gcc/riscv64-unknown-linux-gnu/14.0.0/include/riscv_bitmanip.h:
In function '__riscv_orc_b_32':
/opt/riscv-mainline/lib/gcc/riscv64-unknown-linux-gnu/14.0.0/include/riscv_bitmanip.h:61:10:
error: implicit declaration of function '__builtin_riscv_orc_b_32';
did you mean '__builtin_riscv_orc_b_64'?
[-Wimplicit-function-declaration]
   61 |   return __builtin_riscv_orc_b_32 (x);
  |  ^~~~
  |  __builtin_riscv_orc_b_64

The spec says: Emulated with rev8+sext.w on RV64.
But I think this is a bug in the spec and should be "orc.b + sext.w".
Still, you need to handle that somehow.

$ /opt/riscv-mainline/bin/riscv64-unknown-linux-gnu-gcc
-march=rv64gc_zknd_zkne_zknh_zksed_zksh -mabi=lp64 -mabi=lp64d
/home/cm/src/gcc/riscv-mainline/gcc/testsuite/gcc.target/riscv/scalar_crypto_intrinsic-64.c
/tmp/ccynQLn2.s: Assembler messages:
/tmp/ccynQLn2.s:127: Error: instruction aes64ks1i requires absolute expression
/tmp/ccynQLn2.s:593: Error: instruction sm4ed requires absolute expression
/tmp/ccynQLn2.s:633: Error: instruction sm4ks requires absolute expression

The absolute expression means that you cannot use a variable but must
use an immediate.
E.g.:
uint64_t foo4(uint64_t rs1)
{
return __riscv_aes64ks1i(rs1, 3);
}
Here the 3 will be encoded into the instruction.

There are probably more issues, but I stopped investigating after these two.

Also, there are some missing spaces to separate arguments. E.g.:
  return __riscv_aes64ks1i(rs1,rnum);
...should be...
  return __riscv_aes64ks1i(rs1, rnum);

Please make sure to test these patches for RV32 and RV64 before
sending a new revision.
If you run into issues that you can't resolve, then just reach out.

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-ftypes.def |   2 +
>  gcc/config/riscv/riscv-scalar-crypto.def  |  18 +
>  gcc/config/riscv/riscv_bitmanip.h | 297 +
>  gcc/config/riscv/riscv_crypto.h   | 309 ++
>  .../riscv/scalar_bitmanip_intrinsic-32.c  |  97 ++
>  .../riscv/scalar_bitmanip_intrinsic-64.c  | 115 +++
>  .../riscv/scalar_crypto_intrinsic-32.c| 115 +++
>  .../riscv/scalar_crypto_intrinsic-64.c| 122 +++
>  10 files changed, 1098 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-32.c
>  create mode 100644 
> gcc/testsuite/gcc.target/riscv/scalar_bitmanip_intrinsic-64.c
>  create mode 100644 
> gcc/testsuite/gcc.target/riscv/scalar_crypto_intrinsic-32.c
>  create mode 100644 
> gcc/testsuite/gcc.target/riscv/scalar_crypto_intrinsic-64.c
>
> --
> 2.34.1
>


[PATCH V2 0/2] RISC-V: Add intrinsics for Bitmanip and Scalar Crypto extensions

2023-12-07 Thread Liao Shihua
In accordance with the suggestions of Christoph Müllner, the following 
amendments are made

Update v1 -> v2:
  1. Rename *_intrinsic-* to *_intrinsic-XLEN.
  2. Typo fix.
  3. Intrinsics with immediate arguments will use marcos at O0 .

It's a little patch add just provides a mapping from the RV intrinsics to the 
builtin 
names within GCC.


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-ftypes.def |   2 +
 gcc/config/riscv/riscv-scalar-crypto.def  |  18 +
 gcc/config/riscv/riscv_bitmanip.h | 297 +
 gcc/config/riscv/riscv_crypto.h   | 309 ++
 .../riscv/scalar_bitmanip_intrinsic-32.c  |  97 ++
 .../riscv/scalar_bitmanip_intrinsic-64.c  | 115 +++
 .../riscv/scalar_crypto_intrinsic-32.c| 115 +++
 .../riscv/scalar_crypto_intrinsic-64.c| 122 +++
 10 files changed, 1098 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-32.c
 create mode 100644 
gcc/testsuite/gcc.target/riscv/scalar_bitmanip_intrinsic-64.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/scalar_crypto_intrinsic-32.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/scalar_crypto_intrinsic-64.c

-- 
2.34.1