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