[RFC,v2] RISC-V:Fix th.vsetvli generates from vext_x_v with wrong operand

  From: Yunze Zhu <yunzezhu@linux.alibaba.com>

Fix a bug th.vsetvli generates from vext_x_v with an imm operand,
which reports illegal operand. This patch fix this by replacing
imm operand with reg operand in th.vsetvli.

gcc/ChangeLog:

	* config/riscv/riscv-vsetvl.cc:

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/rvv/xtheadvector/vext_x_v.c: New test.
---
 gcc/config/riscv/riscv-vsetvl.cc                |  2 +-
 .../riscv/rvv/xtheadvector/vext_x_v.c           | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/xtheadvector/vext_x_v.c
  

On Mon, Dec 23, 2024 at 2:52 PM <yunzezhu@linux.alibaba.com> wrote:
>
> From: Yunze Zhu <yunzezhu@linux.alibaba.com>
>
> Fix a bug th.vsetvli generates from vext_x_v with an imm operand,
> which reports illegal operand. This patch fix this by replacing
> imm operand with reg operand in th.vsetvli.
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv-vsetvl.cc:
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/rvv/xtheadvector/vext_x_v.c: New test.
> ---
>  gcc/config/riscv/riscv-vsetvl.cc                |  2 +-
>  .../riscv/rvv/xtheadvector/vext_x_v.c           | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/xtheadvector/vext_x_v.c
>
> diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
> index 720d52964c1c..e6ee074c19fb 100644
> --- a/gcc/config/riscv/riscv-vsetvl.cc
> +++ b/gcc/config/riscv/riscv-vsetvl.cc
> @@ -1186,7 +1186,7 @@ public:
>         set the value of avl to (const_int 0) so that VSETVL PASS will
>         insert vsetvl correctly.*/
>      if (!get_avl ())
> -      avl = GEN_INT (0);
> +      avl = TARGET_XTHEADVECTOR ? gen_rtx_REG (Pmode, 0) : GEN_INT (0);

I feel the semantic has changed here, I checked v-spec 0.7.1 it says
use x0 here means VLMAX rather than real 0,
although it's fine to `th.vext.x.v`, but it just does not seem the
right place to fix?

But don't treat my comment as a blocker since it's vendor specific
logic so it's up to you guys :P

>      rtx sew = gen_int_mode (get_sew (), Pmode);
>      rtx vlmul = gen_int_mode (get_vlmul (), Pmode);
>      rtx ta = gen_int_mode (get_ta (), Pmode);
> diff --git a/gcc/testsuite/gcc.target/riscv/rvv/xtheadvector/vext_x_v.c b/gcc/testsuite/gcc.target/riscv/rvv/xtheadvector/vext_x_v.c
> new file mode 100644
> index 000000000000..be5847727cac
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/rvv/xtheadvector/vext_x_v.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gcxtheadvector -mabi=lp64d -O3 " } */
> +#include <riscv_th_vector.h>
> +
> +int64_t f1 (void * in)
> +{
> +    vint64m1_t v = __riscv_th_vlb_v_i64m1 (in, 2);
> +    vint32m1_t v2 = __riscv_th_vlb_v_i32m1 (in, 2);
> +    int64_t i1 = __riscv_th_vext_x_v_i64m1_i64(v, 2);
> +    int32_t i2 = __riscv_th_vext_x_v_i32m1_i32(v2, 2);
> +    int64_t i = i1 + (int64_t)i2;
> +    return i;
> +}
> +
> +/* { dg-final { scan-assembler-times "th.vsetvli       zero,zero,e32,m1" 1 } } */
> +/* { dg-final { scan-assembler-times "th.vsetvli       zero,zero,e64,m1" 1 } } */
> +/* { dg-final { scan-assembler-not "th.vsetvli zero,0" } } */
> --
> 2.25.1
>
  

On 12/22/24 11:51 PM, yunzezhu@linux.alibaba.com wrote:
> From: Yunze Zhu <yunzezhu@linux.alibaba.com>
> 
> Fix a bug th.vsetvli generates from vext_x_v with an imm operand,
> which reports illegal operand. This patch fix this by replacing
> imm operand with reg operand in th.vsetvli.
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv-vsetvl.cc:
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/rvv/xtheadvector/vext_x_v.c: New test.
I think this was fixed a while back.

The "problem" insns look like this:

> (gdb) p debug_rtx (insn)
> (insn 39 7 13 (parallel [
>             (set (reg:SI 66 vl)
>                 (unspec:SI [
>                         (reg:DI 0 zero)
>                         (const_int 32 [0x20])
>                         (const_int 0 [0])
>                     ] UNSPEC_VSETVL))
>             (set (reg:SI 67 vtype)
>                 (unspec:SI [
>                         (const_int 32 [0x20])
>                         (const_int 0 [0])
>                         (const_int 1 [0x1]) repeated x2
>                     ] UNSPEC_VSETVL))
>         ]) "j.c":8:18 3722 {vsetvl_discard_resultdi}
>      (expr_list:REG_UNUSED (reg:SI 66 vl)
>         (nil)))
Which looks sensible to me.  That's going to output "zero" rather than 
"0", which is what we want.

As I mentioned, I'm iterating with Jinma on a possible case where 
IRA/LRA might be substituting the (const_int 0) form back in.  But as it 
stands I don't see a need for this patch right now.

If you can reproduce this problem on trunk, certainly let me know and 
pass along an updated testcase/args.

Thanks,
Jeff