On 12/14/23 02:48, Christoph Müllner wrote:
On Thu, Dec 14, 2023 at 1:40 AM Jeff Law <jeffreya...@gmail.com> wrote:
On 12/13/23 02:03, Christoph Müllner wrote:
On Wed, Dec 13, 2023 at 9:22 AM Liao Shihua <shi...@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.muell...@vrull.eu>
Tested-by: Christoph Muellner <christoph.muell...@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
