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. 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
