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

[Jeff Law <jeffreyalaw@gmail.com>]

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