RISC-V: fix scalar crypto pattern

Message ID 20231213082244.3760797-1-shihua@iscas.ac.cn
State Superseded
Delegated to: Jeff Law
Headers
Series RISC-V: fix scalar crypto pattern |

Checks

Context Check Description
rivoscibot/toolchain-ci-rivos-lint warning Lint failed
rivoscibot/toolchain-ci-rivos-apply-patch success Patch applied
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gc-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv32gc_zba_zbb_zbc_zbs-ilp32d-non-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gc_zba_zbb_zbc_zbs-lp64d-non-multilib success Build passed
rivoscibot/toolchain-ci-rivos-test fail Testing failed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed

Commit Message

Liao Shihua Dec. 13, 2023, 8:22 a.m. UTC
  In Scalar Crypto Built-In functions, some require immediate parameters,
But register_operand are incorrectly used in the pattern.

E.g.:
   __builtin_riscv_aes64ks1i(rs1,1)
   Before:
      li a5,1
      aes64ks1i a0,a0,a5
      
      Assembler messages:
      Error: instruction aes64ks1i requires absolute expression

   After:
      aes64ks1i a0,a0,1

gcc/ChangeLog:

        * config/riscv/crypto.md: Use immediate_operand instead of register_operand.

gcc/testsuite/ChangeLog:

        * gcc.target/riscv/zknd32.c: Use immediate instead of parameter.
        * gcc.target/riscv/zknd64.c: Ditto.
        * gcc.target/riscv/zkne32.c: Ditto.
        * gcc.target/riscv/zkne64.c: Ditto.
        * gcc.target/riscv/zksed32.c: Ditto.
        * gcc.target/riscv/zksed64.c: Ditto.

---
 gcc/config/riscv/crypto.md               | 16 ++++++++--------
 gcc/testsuite/gcc.target/riscv/zknd32.c  |  8 ++++----
 gcc/testsuite/gcc.target/riscv/zknd64.c  |  4 ++--
 gcc/testsuite/gcc.target/riscv/zkne32.c  |  8 ++++----
 gcc/testsuite/gcc.target/riscv/zkne64.c  |  4 ++--
 gcc/testsuite/gcc.target/riscv/zksed32.c |  8 ++++----
 gcc/testsuite/gcc.target/riscv/zksed64.c |  8 ++++----
 7 files changed, 28 insertions(+), 28 deletions(-)
  

Comments

Christoph Müllner Dec. 13, 2023, 9:03 a.m. UTC | #1
On Wed, Dec 13, 2023 at 9:22 AM Liao Shihua <shihua@iscas.ac.cn> wrote:
>
> In Scalar Crypto Built-In functions, some require immediate parameters,
> But register_operand are incorrectly used in the pattern.
>
> E.g.:
>    __builtin_riscv_aes64ks1i(rs1,1)
>    Before:
>       li a5,1
>       aes64ks1i a0,a0,a5
>
>       Assembler messages:
>       Error: instruction aes64ks1i requires absolute expression
>
>    After:
>       aes64ks1i a0,a0,1

Looks good to me (also tested with rv32 and rv64).
(I was actually surprised that the D03 constraint was not sufficient)

Reviewed-by: Christoph Muellner <christoph.muellner@vrull.eu>
Tested-by: Christoph Muellner <christoph.muellner@vrull.eu>

Nit: I would prefer to separate arguments with a comma followed by a space.
Even if the existing code was not written like that.
E.g. __builtin_riscv_sm4ed(rs1,rs2,1); -> __builtin_riscv_sm4ed(rs1, rs2, 1);

I propose to remove the builtin tests for scalar crypto and scalar bitmanip
as part of the patchset that adds the intrinsic tests (no value in
duplicated tests).

> gcc/ChangeLog:
>
>         * config/riscv/crypto.md: Use immediate_operand instead of register_operand.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/zknd32.c: Use immediate instead of parameter.
>         * gcc.target/riscv/zknd64.c: Ditto.
>         * gcc.target/riscv/zkne32.c: Ditto.
>         * gcc.target/riscv/zkne64.c: Ditto.
>         * gcc.target/riscv/zksed32.c: Ditto.
>         * gcc.target/riscv/zksed64.c: Ditto.
>
> ---
>  gcc/config/riscv/crypto.md               | 16 ++++++++--------
>  gcc/testsuite/gcc.target/riscv/zknd32.c  |  8 ++++----
>  gcc/testsuite/gcc.target/riscv/zknd64.c  |  4 ++--
>  gcc/testsuite/gcc.target/riscv/zkne32.c  |  8 ++++----
>  gcc/testsuite/gcc.target/riscv/zkne64.c  |  4 ++--
>  gcc/testsuite/gcc.target/riscv/zksed32.c |  8 ++++----
>  gcc/testsuite/gcc.target/riscv/zksed64.c |  8 ++++----
>  7 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/gcc/config/riscv/crypto.md b/gcc/config/riscv/crypto.md
> index 03a1d03397d..c45f12e421f 100644
> --- a/gcc/config/riscv/crypto.md
> +++ b/gcc/config/riscv/crypto.md
> @@ -148,7 +148,7 @@
>    [(set (match_operand:SI 0 "register_operand" "=r")
>          (unspec:SI [(match_operand:SI 1 "register_operand" "r")
>                     (match_operand:SI 2 "register_operand" "r")
> -                   (match_operand:SI 3 "register_operand" "D03")]
> +                   (match_operand:SI 3 "immediate_operand" "D03")]
>                     UNSPEC_AES_DSI))]
>    "TARGET_ZKND && !TARGET_64BIT"
>    "aes32dsi\t%0,%1,%2,%3"
> @@ -158,7 +158,7 @@
>    [(set (match_operand:SI 0 "register_operand" "=r")
>          (unspec:SI [(match_operand:SI 1 "register_operand" "r")
>                     (match_operand:SI 2 "register_operand" "r")
> -                   (match_operand:SI 3 "register_operand" "D03")]
> +                   (match_operand:SI 3 "immediate_operand" "D03")]
>                     UNSPEC_AES_DSMI))]
>    "TARGET_ZKND && !TARGET_64BIT"
>    "aes32dsmi\t%0,%1,%2,%3"
> @@ -193,7 +193,7 @@
>  (define_insn "riscv_aes64ks1i"
>    [(set (match_operand:DI 0 "register_operand" "=r")
>          (unspec:DI [(match_operand:DI 1 "register_operand" "r")
> -                   (match_operand:SI 2 "register_operand" "DsA")]
> +                   (match_operand:SI 2 "immediate_operand" "DsA")]
>                     UNSPEC_AES_KS1I))]
>    "(TARGET_ZKND || TARGET_ZKNE) && TARGET_64BIT"
>    "aes64ks1i\t%0,%1,%2"
> @@ -214,7 +214,7 @@
>    [(set (match_operand:SI 0 "register_operand" "=r")
>          (unspec:SI [(match_operand:SI 1 "register_operand" "r")
>                     (match_operand:SI 2 "register_operand" "r")
> -                   (match_operand:SI 3 "register_operand" "D03")]
> +                   (match_operand:SI 3 "immediate_operand" "D03")]
>                     UNSPEC_AES_ESI))]
>    "TARGET_ZKNE && !TARGET_64BIT"
>    "aes32esi\t%0,%1,%2,%3"
> @@ -224,7 +224,7 @@
>    [(set (match_operand:SI 0 "register_operand" "=r")
>          (unspec:SI [(match_operand:SI 1 "register_operand" "r")
>                     (match_operand:SI 2 "register_operand" "r")
> -                   (match_operand:SI 3 "register_operand" "D03")]
> +                   (match_operand:SI 3 "immediate_operand" "D03")]
>                     UNSPEC_AES_ESMI))]
>    "TARGET_ZKNE && !TARGET_64BIT"
>    "aes32esmi\t%0,%1,%2,%3"
> @@ -431,7 +431,7 @@
>    [(set (match_operand:SI 0 "register_operand" "=r")
>          (unspec:SI [(match_operand:SI 1 "register_operand" "r")
>                     (match_operand:SI 2 "register_operand" "r")
> -                   (match_operand:SI 3 "register_operand" "D03")]
> +                   (match_operand:SI 3 "immediate_operand" "D03")]
>                     SM4_OP))]
>    "TARGET_ZKSED && !TARGET_64BIT"
>    "<sm4_op>\t%0,%1,%2,%3"
> @@ -442,7 +442,7 @@
>          (sign_extend:DI
>               (unspec:SI [(match_operand:SI 1 "register_operand" "r")
>                          (match_operand:SI 2 "register_operand" "r")
> -                        (match_operand:SI 3 "register_operand" "D03")]
> +                        (match_operand:SI 3 "immediate_operand" "D03")]
>                          SM4_OP)))]
>    "TARGET_ZKSED && TARGET_64BIT"
>    "<sm4_op>\t%0,%1,%2,%3"
> @@ -452,7 +452,7 @@
>    [(set (match_operand:SI 0 "register_operand" "=r")
>          (unspec:SI [(match_operand:SI 1 "register_operand" "r")
>                     (match_operand:SI 2 "register_operand" "r")
> -                   (match_operand:SI 3 "register_operand" "D03")]
> +                   (match_operand:SI 3 "immediate_operand" "D03")]
>                     SM4_OP))]
>    "TARGET_ZKSED"
>    {
> diff --git a/gcc/testsuite/gcc.target/riscv/zknd32.c b/gcc/testsuite/gcc.target/riscv/zknd32.c
> index e60c027e091..9711b120001 100644
> --- a/gcc/testsuite/gcc.target/riscv/zknd32.c
> +++ b/gcc/testsuite/gcc.target/riscv/zknd32.c
> @@ -4,14 +4,14 @@
>
>  #include <stdint-gcc.h>
>
> -uint32_t foo1(uint32_t rs1, uint32_t rs2, int bs)
> +uint32_t foo1(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_aes32dsi(rs1,rs2,bs);
> +    return __builtin_riscv_aes32dsi(rs1,rs2,1);
>  }
>
> -uint32_t foo2(uint32_t rs1, uint32_t rs2, int bs)
> +uint32_t foo2(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_aes32dsmi(rs1,rs2,bs);
> +    return __builtin_riscv_aes32dsmi(rs1,rs2,1);
>  }
>
>  /* { dg-final { scan-assembler-times "aes32dsi" 1 } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/zknd64.c b/gcc/testsuite/gcc.target/riscv/zknd64.c
> index 707418cd51e..d56c03f201e 100644
> --- a/gcc/testsuite/gcc.target/riscv/zknd64.c
> +++ b/gcc/testsuite/gcc.target/riscv/zknd64.c
> @@ -14,9 +14,9 @@ uint64_t foo2(uint64_t rs1, uint64_t rs2)
>      return __builtin_riscv_aes64dsm(rs1,rs2);
>  }
>
> -uint64_t foo3(uint64_t rs1, unsigned rnum)
> +uint64_t foo3(uint64_t rs1)
>  {
> -    return __builtin_riscv_aes64ks1i(rs1,rnum);
> +    return __builtin_riscv_aes64ks1i(rs1,1);
>  }
>
>  uint64_t foo4(uint64_t rs1, uint64_t rs2)
> diff --git a/gcc/testsuite/gcc.target/riscv/zkne32.c b/gcc/testsuite/gcc.target/riscv/zkne32.c
> index 252e9ffa43b..378c3a2fdd3 100644
> --- a/gcc/testsuite/gcc.target/riscv/zkne32.c
> +++ b/gcc/testsuite/gcc.target/riscv/zkne32.c
> @@ -4,14 +4,14 @@
>
>  #include <stdint-gcc.h>
>
> -uint32_t foo1(uint32_t rs1, uint32_t rs2, unsigned bs)
> +uint32_t foo1(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_aes32esi(rs1, rs2, bs);
> +    return __builtin_riscv_aes32esi(rs1, rs2,1);
>  }
>
> -uint32_t foo2(uint32_t rs1, uint32_t rs2, unsigned bs)
> +uint32_t foo2(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_aes32esmi(rs1, rs2, bs);
> +    return __builtin_riscv_aes32esmi(rs1, rs2, 1);
>  }
>
>  /* { dg-final { scan-assembler-times "aes32esi" 1 } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/zkne64.c b/gcc/testsuite/gcc.target/riscv/zkne64.c
> index b25f6b5c29a..d5435b399c6 100644
> --- a/gcc/testsuite/gcc.target/riscv/zkne64.c
> +++ b/gcc/testsuite/gcc.target/riscv/zkne64.c
> @@ -14,9 +14,9 @@ uint64_t foo2(uint64_t rs1, uint64_t rs2)
>      return __builtin_riscv_aes64esm(rs1,rs2);
>  }
>
> -uint64_t foo3(uint64_t rs1, unsigned rnum)
> +uint64_t foo3(uint64_t rs1)
>  {
> -    return __builtin_riscv_aes64ks1i(rs1,rnum);
> +    return __builtin_riscv_aes64ks1i(rs1,1);
>  }
>
>  uint64_t foo4(uint64_t rs1, uint64_t rs2)
> diff --git a/gcc/testsuite/gcc.target/riscv/zksed32.c b/gcc/testsuite/gcc.target/riscv/zksed32.c
> index 0e8f01cd548..a3583d9f4ae 100644
> --- a/gcc/testsuite/gcc.target/riscv/zksed32.c
> +++ b/gcc/testsuite/gcc.target/riscv/zksed32.c
> @@ -4,14 +4,14 @@
>
>  #include <stdint-gcc.h>
>
> -uint32_t foo1(uint32_t rs1, uint32_t rs2, unsigned bs)
> +uint32_t foo1(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_sm4ks(rs1,rs2,bs);
> +    return __builtin_riscv_sm4ks(rs1,rs2,1);
>  }
>
> -uint32_t foo2(uint32_t rs1, uint32_t rs2, unsigned bs)
> +uint32_t foo2(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_sm4ed(rs1,rs2,bs);
> +    return __builtin_riscv_sm4ed(rs1,rs2,1);
>  }
>
>
> diff --git a/gcc/testsuite/gcc.target/riscv/zksed64.c b/gcc/testsuite/gcc.target/riscv/zksed64.c
> index 9e4d1961419..9b06e47ce70 100644
> --- a/gcc/testsuite/gcc.target/riscv/zksed64.c
> +++ b/gcc/testsuite/gcc.target/riscv/zksed64.c
> @@ -4,14 +4,14 @@
>
>  #include <stdint-gcc.h>
>
> -uint32_t foo1(uint32_t rs1, uint32_t rs2, unsigned bs)
> +uint32_t foo1(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_sm4ks(rs1,rs2,bs);
> +    return __builtin_riscv_sm4ks(rs1,rs2,1);
>  }
>
> -uint32_t foo2(uint32_t rs1, uint32_t rs2, unsigned bs)
> +uint32_t foo2(uint32_t rs1, uint32_t rs2)
>  {
> -    return __builtin_riscv_sm4ed(rs1,rs2,bs);
> +    return __builtin_riscv_sm4ed(rs1,rs2,1);
>  }
>
>
> --
> 2.34.1
>
  
Jeff Law Dec. 14, 2023, 12:40 a.m. UTC | #2
On 12/13/23 02:03, Christoph Müllner wrote:
> On Wed, Dec 13, 2023 at 9:22 AM Liao Shihua <shihua@iscas.ac.cn> wrote:
>>
>> In Scalar Crypto Built-In functions, some require immediate parameters,
>> But register_operand are incorrectly used in the pattern.
>>
>> E.g.:
>>     __builtin_riscv_aes64ks1i(rs1,1)
>>     Before:
>>        li a5,1
>>        aes64ks1i a0,a0,a5
>>
>>        Assembler messages:
>>        Error: instruction aes64ks1i requires absolute expression
>>
>>     After:
>>        aes64ks1i a0,a0,1
> 
> Looks good to me (also tested with rv32 and rv64).
> (I was actually surprised that the D03 constraint was not sufficient)
> 
> Reviewed-by: Christoph Muellner <christoph.muellner@vrull.eu>
> Tested-by: Christoph Muellner <christoph.muellner@vrull.eu>
> 
> Nit: I would prefer to separate arguments with a comma followed by a space.
> Even if the existing code was not written like that.
> E.g. __builtin_riscv_sm4ed(rs1,rs2,1); -> __builtin_riscv_sm4ed(rs1, rs2, 1);
> 
> I propose to remove the builtin tests for scalar crypto and scalar bitmanip
> as part of the patchset that adds the intrinsic tests (no value in
> duplicated tests).
> 
>> gcc/ChangeLog:
>>
>>          * config/riscv/crypto.md: Use immediate_operand instead of register_operand.
You should mention the actual patterns changed.

I would strongly recommend adding some tests that out of range cases are 
rejected (out of range constants as well as a variable for that last 
argument).  I did that in my patch from June to fix this problem (which 
was never acked/reviewed).

Jeff
  
Christoph Müllner Dec. 14, 2023, 9:48 a.m. UTC | #3
On Thu, Dec 14, 2023 at 1:40 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
> On 12/13/23 02:03, Christoph Müllner wrote:
> > On Wed, Dec 13, 2023 at 9:22 AM Liao Shihua <shihua@iscas.ac.cn> wrote:
> >>
> >> In Scalar Crypto Built-In functions, some require immediate parameters,
> >> But register_operand are incorrectly used in the pattern.
> >>
> >> E.g.:
> >>     __builtin_riscv_aes64ks1i(rs1,1)
> >>     Before:
> >>        li a5,1
> >>        aes64ks1i a0,a0,a5
> >>
> >>        Assembler messages:
> >>        Error: instruction aes64ks1i requires absolute expression
> >>
> >>     After:
> >>        aes64ks1i a0,a0,1
> >
> > Looks good to me (also tested with rv32 and rv64).
> > (I was actually surprised that the D03 constraint was not sufficient)
> >
> > Reviewed-by: Christoph Muellner <christoph.muellner@vrull.eu>
> > Tested-by: Christoph Muellner <christoph.muellner@vrull.eu>
> >
> > Nit: I would prefer to separate arguments with a comma followed by a space.
> > Even if the existing code was not written like that.
> > E.g. __builtin_riscv_sm4ed(rs1,rs2,1); -> __builtin_riscv_sm4ed(rs1, rs2, 1);
> >
> > I propose to remove the builtin tests for scalar crypto and scalar bitmanip
> > as part of the patchset that adds the intrinsic tests (no value in
> > duplicated tests).
> >
> >> gcc/ChangeLog:
> >>
> >>          * config/riscv/crypto.md: Use immediate_operand instead of register_operand.
> You should mention the actual patterns changed.
>
> I would strongly recommend adding some tests that out of range cases are
> rejected (out of range constants as well as a variable for that last
> argument).  I did that in my patch from June to fix this problem (which
> was never acked/reviewed).

Sorry, I was not aware of this patch.
Since Jeff's patch was here first and also includes more tests, I
propose to move forward with his patch (but I'm not a maintainer!).
Therefore, I've reviewed Jeff's patch and replied to his email.

FWIW: Jeff's patch can be found here:
  https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622233.html
  
Liao Shihua Dec. 14, 2023, 11:12 a.m. UTC | #4
> Sorry, I was not aware of this patch.
> Since Jeff's patch was here first and also includes more tests, I
> propose to move forward with his patch (but I'm not a maintainer!).
> Therefore, I've reviewed Jeff's patch and replied to his email.
>
> FWIW: Jeff's patch can be found here:
>    https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622233.html

No problem.

And I would tend to remove the D03 constraint if we used const_0_3_operand.

BR

Liao  Shihua
  
Jeff Law Dec. 14, 2023, 2:57 p.m. UTC | #5
On 12/14/23 02:48, Christoph Müllner wrote:
> On Thu, Dec 14, 2023 at 1:40 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>> On 12/13/23 02:03, Christoph Müllner wrote:
>>> On Wed, Dec 13, 2023 at 9:22 AM Liao Shihua <shihua@iscas.ac.cn> wrote:
>>>>
>>>> In Scalar Crypto Built-In functions, some require immediate parameters,
>>>> But register_operand are incorrectly used in the pattern.
>>>>
>>>> E.g.:
>>>>      __builtin_riscv_aes64ks1i(rs1,1)
>>>>      Before:
>>>>         li a5,1
>>>>         aes64ks1i a0,a0,a5
>>>>
>>>>         Assembler messages:
>>>>         Error: instruction aes64ks1i requires absolute expression
>>>>
>>>>      After:
>>>>         aes64ks1i a0,a0,1
>>>
>>> Looks good to me (also tested with rv32 and rv64).
>>> (I was actually surprised that the D03 constraint was not sufficient)
>>>
>>> Reviewed-by: Christoph Muellner <christoph.muellner@vrull.eu>
>>> Tested-by: Christoph Muellner <christoph.muellner@vrull.eu>
>>>
>>> Nit: I would prefer to separate arguments with a comma followed by a space.
>>> Even if the existing code was not written like that.
>>> E.g. __builtin_riscv_sm4ed(rs1,rs2,1); -> __builtin_riscv_sm4ed(rs1, rs2, 1);
>>>
>>> I propose to remove the builtin tests for scalar crypto and scalar bitmanip
>>> as part of the patchset that adds the intrinsic tests (no value in
>>> duplicated tests).
>>>
>>>> gcc/ChangeLog:
>>>>
>>>>           * config/riscv/crypto.md: Use immediate_operand instead of register_operand.
>> You should mention the actual patterns changed.
>>
>> I would strongly recommend adding some tests that out of range cases are
>> rejected (out of range constants as well as a variable for that last
>> argument).  I did that in my patch from June to fix this problem (which
>> was never acked/reviewed).
> 
> Sorry, I was not aware of this patch.
> Since Jeff's patch was here first and also includes more tests, I
> propose to move forward with his patch (but I'm not a maintainer!).
> Therefore, I've reviewed Jeff's patch and replied to his email.
> 
> FWIW: Jeff's patch can be found here:
>    https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622233.html
So my patch will need a trivial update as a couple patterns changed to 
use an iterator and thus the patch context doesn't match anymore.  It's 
a trivial fix.

jeff
  
Jeff Law Dec. 14, 2023, 7:30 p.m. UTC | #6
On 12/14/23 02:48, Christoph Müllner wrote:
> On Thu, Dec 14, 2023 at 1:40 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>> On 12/13/23 02:03, Christoph Müllner wrote:
>>> On Wed, Dec 13, 2023 at 9:22 AM Liao Shihua <shihua@iscas.ac.cn> wrote:
>>>>
>>>> In Scalar Crypto Built-In functions, some require immediate parameters,
>>>> But register_operand are incorrectly used in the pattern.
>>>>
>>>> E.g.:
>>>>      __builtin_riscv_aes64ks1i(rs1,1)
>>>>      Before:
>>>>         li a5,1
>>>>         aes64ks1i a0,a0,a5
>>>>
>>>>         Assembler messages:
>>>>         Error: instruction aes64ks1i requires absolute expression
>>>>
>>>>      After:
>>>>         aes64ks1i a0,a0,1
>>>
>>> Looks good to me (also tested with rv32 and rv64).
>>> (I was actually surprised that the D03 constraint was not sufficient)
>>>
>>> Reviewed-by: Christoph Muellner <christoph.muellner@vrull.eu>
>>> Tested-by: Christoph Muellner <christoph.muellner@vrull.eu>
>>>
>>> Nit: I would prefer to separate arguments with a comma followed by a space.
>>> Even if the existing code was not written like that.
>>> E.g. __builtin_riscv_sm4ed(rs1,rs2,1); -> __builtin_riscv_sm4ed(rs1, rs2, 1);
>>>
>>> I propose to remove the builtin tests for scalar crypto and scalar bitmanip
>>> as part of the patchset that adds the intrinsic tests (no value in
>>> duplicated tests).
>>>
>>>> gcc/ChangeLog:
>>>>
>>>>           * config/riscv/crypto.md: Use immediate_operand instead of register_operand.
>> You should mention the actual patterns changed.
>>
>> I would strongly recommend adding some tests that out of range cases are
>> rejected (out of range constants as well as a variable for that last
>> argument).  I did that in my patch from June to fix this problem (which
>> was never acked/reviewed).
> 
> Sorry, I was not aware of this patch.
No worries.  I'd planned to ping it again as part of the stage3 
bugfixing effort ;-)  It wasn't until I started looking at Liao's patch 
that I realized he was fixing the same problem.

> Since Jeff's patch was here first and also includes more tests, I
> propose to move forward with his patch (but I'm not a maintainer!).
> Therefore, I've reviewed Jeff's patch and replied to his email.
Thanks.  I think the combination of your review, the high overlap with 
Liao's work and my status as a global maintainer should be sufficient to 
move this forward.


Jeff
  
Jeff Law Dec. 14, 2023, 7:31 p.m. UTC | #7
On 12/14/23 04:12, Liao Shihua wrote:
>> Sorry, I was not aware of this patch.
>> Since Jeff's patch was here first and also includes more tests, I
>> propose to move forward with his patch (but I'm not a maintainer!).
>> Therefore, I've reviewed Jeff's patch and replied to his email.
>>
>> FWIW: Jeff's patch can be found here:
>>    https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622233.html
> 
> No problem.
> 
> And I would tend to remove the D03 constraint if we used const_0_3_operand.
Seems reasonable to me.  I'll remove it.

jeff
  

Patch

diff --git a/gcc/config/riscv/crypto.md b/gcc/config/riscv/crypto.md
index 03a1d03397d..c45f12e421f 100644
--- a/gcc/config/riscv/crypto.md
+++ b/gcc/config/riscv/crypto.md
@@ -148,7 +148,7 @@ 
   [(set (match_operand:SI 0 "register_operand" "=r")
         (unspec:SI [(match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")
-                   (match_operand:SI 3 "register_operand" "D03")]
+                   (match_operand:SI 3 "immediate_operand" "D03")]
                    UNSPEC_AES_DSI))]
   "TARGET_ZKND && !TARGET_64BIT"
   "aes32dsi\t%0,%1,%2,%3"
@@ -158,7 +158,7 @@ 
   [(set (match_operand:SI 0 "register_operand" "=r")
         (unspec:SI [(match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")
-                   (match_operand:SI 3 "register_operand" "D03")]
+                   (match_operand:SI 3 "immediate_operand" "D03")]
                    UNSPEC_AES_DSMI))]
   "TARGET_ZKND && !TARGET_64BIT"
   "aes32dsmi\t%0,%1,%2,%3"
@@ -193,7 +193,7 @@ 
 (define_insn "riscv_aes64ks1i"
   [(set (match_operand:DI 0 "register_operand" "=r")
         (unspec:DI [(match_operand:DI 1 "register_operand" "r")
-                   (match_operand:SI 2 "register_operand" "DsA")]
+                   (match_operand:SI 2 "immediate_operand" "DsA")]
                    UNSPEC_AES_KS1I))]
   "(TARGET_ZKND || TARGET_ZKNE) && TARGET_64BIT"
   "aes64ks1i\t%0,%1,%2"
@@ -214,7 +214,7 @@ 
   [(set (match_operand:SI 0 "register_operand" "=r")
         (unspec:SI [(match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")
-                   (match_operand:SI 3 "register_operand" "D03")]
+                   (match_operand:SI 3 "immediate_operand" "D03")]
                    UNSPEC_AES_ESI))]
   "TARGET_ZKNE && !TARGET_64BIT"
   "aes32esi\t%0,%1,%2,%3"
@@ -224,7 +224,7 @@ 
   [(set (match_operand:SI 0 "register_operand" "=r")
         (unspec:SI [(match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")
-                   (match_operand:SI 3 "register_operand" "D03")]
+                   (match_operand:SI 3 "immediate_operand" "D03")]
                    UNSPEC_AES_ESMI))]
   "TARGET_ZKNE && !TARGET_64BIT"
   "aes32esmi\t%0,%1,%2,%3"
@@ -431,7 +431,7 @@ 
   [(set (match_operand:SI 0 "register_operand" "=r")
         (unspec:SI [(match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")
-                   (match_operand:SI 3 "register_operand" "D03")]
+                   (match_operand:SI 3 "immediate_operand" "D03")]
                    SM4_OP))]
   "TARGET_ZKSED && !TARGET_64BIT"
   "<sm4_op>\t%0,%1,%2,%3"
@@ -442,7 +442,7 @@ 
         (sign_extend:DI
              (unspec:SI [(match_operand:SI 1 "register_operand" "r")
                         (match_operand:SI 2 "register_operand" "r")
-                        (match_operand:SI 3 "register_operand" "D03")]
+                        (match_operand:SI 3 "immediate_operand" "D03")]
                         SM4_OP)))]
   "TARGET_ZKSED && TARGET_64BIT"
   "<sm4_op>\t%0,%1,%2,%3"
@@ -452,7 +452,7 @@ 
   [(set (match_operand:SI 0 "register_operand" "=r")
         (unspec:SI [(match_operand:SI 1 "register_operand" "r")
                    (match_operand:SI 2 "register_operand" "r")
-                   (match_operand:SI 3 "register_operand" "D03")]
+                   (match_operand:SI 3 "immediate_operand" "D03")]
                    SM4_OP))]
   "TARGET_ZKSED"
   {
diff --git a/gcc/testsuite/gcc.target/riscv/zknd32.c b/gcc/testsuite/gcc.target/riscv/zknd32.c
index e60c027e091..9711b120001 100644
--- a/gcc/testsuite/gcc.target/riscv/zknd32.c
+++ b/gcc/testsuite/gcc.target/riscv/zknd32.c
@@ -4,14 +4,14 @@ 
 
 #include <stdint-gcc.h>
 
-uint32_t foo1(uint32_t rs1, uint32_t rs2, int bs)
+uint32_t foo1(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_aes32dsi(rs1,rs2,bs);
+    return __builtin_riscv_aes32dsi(rs1,rs2,1);
 }
 
-uint32_t foo2(uint32_t rs1, uint32_t rs2, int bs)
+uint32_t foo2(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_aes32dsmi(rs1,rs2,bs);
+    return __builtin_riscv_aes32dsmi(rs1,rs2,1);
 }
 
 /* { dg-final { scan-assembler-times "aes32dsi" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zknd64.c b/gcc/testsuite/gcc.target/riscv/zknd64.c
index 707418cd51e..d56c03f201e 100644
--- a/gcc/testsuite/gcc.target/riscv/zknd64.c
+++ b/gcc/testsuite/gcc.target/riscv/zknd64.c
@@ -14,9 +14,9 @@  uint64_t foo2(uint64_t rs1, uint64_t rs2)
     return __builtin_riscv_aes64dsm(rs1,rs2);
 }
 
-uint64_t foo3(uint64_t rs1, unsigned rnum)
+uint64_t foo3(uint64_t rs1)
 {
-    return __builtin_riscv_aes64ks1i(rs1,rnum);
+    return __builtin_riscv_aes64ks1i(rs1,1);
 }
 
 uint64_t foo4(uint64_t rs1, uint64_t rs2)
diff --git a/gcc/testsuite/gcc.target/riscv/zkne32.c b/gcc/testsuite/gcc.target/riscv/zkne32.c
index 252e9ffa43b..378c3a2fdd3 100644
--- a/gcc/testsuite/gcc.target/riscv/zkne32.c
+++ b/gcc/testsuite/gcc.target/riscv/zkne32.c
@@ -4,14 +4,14 @@ 
 
 #include <stdint-gcc.h>
 
-uint32_t foo1(uint32_t rs1, uint32_t rs2, unsigned bs)
+uint32_t foo1(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_aes32esi(rs1, rs2, bs);
+    return __builtin_riscv_aes32esi(rs1, rs2,1);
 }
 
-uint32_t foo2(uint32_t rs1, uint32_t rs2, unsigned bs)
+uint32_t foo2(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_aes32esmi(rs1, rs2, bs);
+    return __builtin_riscv_aes32esmi(rs1, rs2, 1);
 }
 
 /* { dg-final { scan-assembler-times "aes32esi" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zkne64.c b/gcc/testsuite/gcc.target/riscv/zkne64.c
index b25f6b5c29a..d5435b399c6 100644
--- a/gcc/testsuite/gcc.target/riscv/zkne64.c
+++ b/gcc/testsuite/gcc.target/riscv/zkne64.c
@@ -14,9 +14,9 @@  uint64_t foo2(uint64_t rs1, uint64_t rs2)
     return __builtin_riscv_aes64esm(rs1,rs2);
 }
 
-uint64_t foo3(uint64_t rs1, unsigned rnum)
+uint64_t foo3(uint64_t rs1)
 {
-    return __builtin_riscv_aes64ks1i(rs1,rnum);
+    return __builtin_riscv_aes64ks1i(rs1,1);
 }
 
 uint64_t foo4(uint64_t rs1, uint64_t rs2)
diff --git a/gcc/testsuite/gcc.target/riscv/zksed32.c b/gcc/testsuite/gcc.target/riscv/zksed32.c
index 0e8f01cd548..a3583d9f4ae 100644
--- a/gcc/testsuite/gcc.target/riscv/zksed32.c
+++ b/gcc/testsuite/gcc.target/riscv/zksed32.c
@@ -4,14 +4,14 @@ 
 
 #include <stdint-gcc.h>
 
-uint32_t foo1(uint32_t rs1, uint32_t rs2, unsigned bs)
+uint32_t foo1(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_sm4ks(rs1,rs2,bs);
+    return __builtin_riscv_sm4ks(rs1,rs2,1);
 }
 
-uint32_t foo2(uint32_t rs1, uint32_t rs2, unsigned bs)
+uint32_t foo2(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_sm4ed(rs1,rs2,bs);
+    return __builtin_riscv_sm4ed(rs1,rs2,1);
 }
 
 
diff --git a/gcc/testsuite/gcc.target/riscv/zksed64.c b/gcc/testsuite/gcc.target/riscv/zksed64.c
index 9e4d1961419..9b06e47ce70 100644
--- a/gcc/testsuite/gcc.target/riscv/zksed64.c
+++ b/gcc/testsuite/gcc.target/riscv/zksed64.c
@@ -4,14 +4,14 @@ 
 
 #include <stdint-gcc.h>
 
-uint32_t foo1(uint32_t rs1, uint32_t rs2, unsigned bs)
+uint32_t foo1(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_sm4ks(rs1,rs2,bs);
+    return __builtin_riscv_sm4ks(rs1,rs2,1);
 }
 
-uint32_t foo2(uint32_t rs1, uint32_t rs2, unsigned bs)
+uint32_t foo2(uint32_t rs1, uint32_t rs2)
 {
-    return __builtin_riscv_sm4ed(rs1,rs2,bs);
+    return __builtin_riscv_sm4ed(rs1,rs2,1);
 }