Thanks Jeff for comments.
> So a bit of high level background why this is needed would be helpful.
I see. The problem comes from the gen_lowpart when passing the args to SAT_SUB
directly(aka without func args).
SAT_SUB with args, we have input rtx (subreg/s/u:QI (reg/v:DI 135 [ x ]) 0),
and then gen_lowpart will convert to (reg/v:DI 135 [ x ]).
SAT_SUB without args, we have input rtx (reg:QI 141), and then gen_lowpar will
convert to subreg:DI (reg:QI 141) 0).
Unfortunately we don't have sub/add for QImode in scalar, thus we need to sign
extend to Xmode to ensure the correctness.
For example, for 0xff(-1 for QImode) sub 0x1(1 for QImode), we actually want to
-1 - 1 = -2 but when Xmode sub, we will have 0xff - 1 = 0xfe.
Thus, we need to sign extend 0xff (Qmode) to 0xffffffffffffffff(assume XImode
is DImode) for sub.
I will add more descriptions about why from the commit log.
> You might be able to (ab)use riscv_expand_comparands rather than making
> a new function. You'd probably want to rename it in that case.
Just would like wrap the mess in a separate function to keep expand func
simple. It also need to take care of const_int rtx later
Similar to riscv_gen_zero_extend_rtx. It is a separated refactor, thus I didn't
involve that for this bug fix.
> The other high level question, wouldn't this be needed for other signed
> cases, not just those going through expand_sssub?
I think both the ssadd and sstrunc may need this. To be efficient, I send the
sssub bugfix first, and then validate the ssadd and sstrunc
in the meantime.
Pan
-----Original Message-----
From: Jeff Law <jeffreya...@gmail.com>
Sent: Wednesday, January 22, 2025 6:46 AM
To: Li, Pan2 <pan2...@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zh...@rivai.ai; kito.ch...@gmail.com; rdapp....@gmail.com
Subject: Re: [PATCH v1] RISC-V: Fix incorrect code gen for scalar signed
SAT_SUB [PR117688]
On 1/20/25 2:18 AM, pan2...@intel.com wrote:
> From: Pan Li <pan2...@intel.com>
>
> This patch would like to fix the wroing code generation for the scalar
> signed SAT_SUB. The input can be QI/HI/SI/DI while the alu like sub
> can only work on Xmode, thus we need to make sure the value of input
> are well signed-extended at first. But the gen_lowpart will generate
> something like lbu which will perform the zero extended.
>
> The below test suites are passed for this patch.
> * The rv64gcv fully regression test.
>
> Note we also notice some refinement like to support const_int for input
> or similar issue for ssadd and/or sstruct. But we would like to fix
> it by another patch(es).
>
> PR target/117688
>
> gcc/ChangeLog:
>
> * config/riscv/riscv.cc (riscv_gen_sign_extend_rtx): Add new func
> to make sure the op is signed extended to Xmode.
> (riscv_expand_sssub): Leverage above func to perform sign extend
> if not Xmode.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/pr117688-run-1-s16.c: New test.
> * gcc.target/riscv/pr117688-run-1-s32.c: New test.
> * gcc.target/riscv/pr117688-run-1-s8.c: New test.
> * gcc.target/riscv/pr117688.h: New test.
So a bit of high level background why this is needed would be helpful.
My suspicion is that we're using a functional interface, but without
going through any of the usual parameter/argument setup routines. So a
sub-word object hasn't gone through a mandatory sign extension.
>
> +/* Generate a REG rtx of Xmode from the given rtx and mode.
> + The rtx x can be REG (QI/HI/SI/DI).
> + The machine_mode mode is the original mode from define pattern.
> +
> + If rtx is REG and Xmode, the RTX x will be returned directly.
> +
> + If rtx is REG and non-Xmode, the sign extended to new REG of Xmode will be
> + returned.
> +
> + Then the underlying expanding can perform the code generation based on
> + the REG rtx of Xmode, instead of taking care of these in expand func. */
> +
> +static rtx
> +riscv_gen_sign_extend_rtx (rtx x, machine_mode mode)
> +{
> + if (mode == Xmode)
> + return x;
> +
> + rtx xmode_reg = gen_reg_rtx (Xmode);
> + riscv_emit_unary (SIGN_EXTEND, xmode_reg, x);
> +
> + return xmode_reg;
> +}
You might be able to (ab)use riscv_expand_comparands rather than making
a new function. You'd probably want to rename it in that case.
The other high level question, wouldn't this be needed for other signed
cases, not just those going through expand_sssub?
jeff
