Re: [PATCH] RISC-V: fix scalar crypto pattern

[Jeff Law <jeffreyalaw@gmail.com>]

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