Re: Re: [PATCH] RISC-V: Fix bug reported by PR109535
Thanks Jeff. Address Jeff's comment and resend fix patch: https://gcc.gnu.org/pipermail/gcc-patches/2023-April/616170.html This patch also added a testcase coming from Kito (Kito reduced google/highway testcase from over 10W lines codes into 100 lines codes!!!). juzhe.zh...@rivai.ai From: Jeff Law Date: 2023-04-19 13:43 To: juzhe.zh...@rivai.ai; kito.cheng; Richard Biener CC: gcc-patches; palmer Subject: Re: [PATCH] RISC-V: Fix bug reported by PR109535 On 4/18/23 19:29, juzhe.zh...@rivai.ai wrote: > I tried refers_to_regno_p > It can not work for us since it just return true or false whether the > "rtx" has the regno. Use refers_to_regno_p instead of the equality comparison for the REGNO. So you're still going to have count_regno_occurrences, you're just changing the test it uses so that it works for modes which potentially span multiple hard registers. Note that you'll want to pass in AVL rather than REGNO (avl). When you call refers_to_regno_p it'll look something like tmp = REGNO (avl); mode = GET_MODE (avl); if (REG_P (recog_data.operand[i]) && refers_to_regno_p (tmp, hard_regno_nregs (tmp, mode), recog_data.operand[i], NULL)) Or something like that. I'm assuming AVL is a hard register at this point. If it could be a pseudo the code will be slightly different. I'm still not sure all this stuff is handling SUBREGs properly either. Though if it's only checked after reload, we should be OK as we should have simplified the subreg away. Jeff
Re: [PATCH] RISC-V: Fix bug reported by PR109535
On 4/18/23 19:29, juzhe.zh...@rivai.ai wrote: I tried refers_to_regno_p It can not work for us since it just return true or false whether the "rtx" has the regno. Use refers_to_regno_p instead of the equality comparison for the REGNO. So you're still going to have count_regno_occurrences, you're just changing the test it uses so that it works for modes which potentially span multiple hard registers. Note that you'll want to pass in AVL rather than REGNO (avl). When you call refers_to_regno_p it'll look something like tmp = REGNO (avl); mode = GET_MODE (avl); if (REG_P (recog_data.operand[i]) && refers_to_regno_p (tmp, hard_regno_nregs (tmp, mode), recog_data.operand[i], NULL)) Or something like that. I'm assuming AVL is a hard register at this point. If it could be a pseudo the code will be slightly different. I'm still not sure all this stuff is handling SUBREGs properly either. Though if it's only checked after reload, we should be OK as we should have simplified the subreg away. Jeff
Re: Re: [PATCH] RISC-V: Fix bug reported by PR109535
Meaning when "AVL" is a reg and appears once, we will eliminate "AVL" operand in uses. If it appears more than once, we don't eliminate the "AVL" operand in uses. You can this case: vint8m1_t b = __riscv_vadd_vx_i8m1 (a, vl, vl); Here you can see "vl" variable not only serves as the "AVL" which is used in vsetvli but also it serves as "scalar operand" involved in the vadd.vx operation. In this case, we can eliminate the operand "vl" However, vint8m1_t b = __riscv_vadd_vx_i8m1 (a, x, vl); This case you can see "vl" operand only serves as "avl" which is used already in vsetvli instructions before, so this operand is not used anymore in "vadd.vx" instruction, I removed this operand and dependency. Feel free to give me more comments. Thanks. juzhe.zh...@rivai.ai From: Jeff Law Date: 2023-04-19 09:11 To: juzhe.zh...@rivai.ai; kito.cheng; Richard Biener CC: gcc-patches; palmer Subject: Re: [PATCH] RISC-V: Fix bug reported by PR109535 On 4/18/23 19:04, juzhe.zh...@rivai.ai wrote: > The bug issue reported by google/highway project: > (set(..) > (reg:QI s0) > (reg:DI s0)) > > The "avl" operand rtx = (reg:DI s0) > count_occurrences return 1 however the actual regno occurrences should be 2. > In this case, the VSETVL PASS will eliminate the use of (reg:DI s0) then > file assertion in RTL_SSA. > Instead, we should not eliminate "s0" dependency. So these are not vector hard registers, but GPR hard registers. Meaning you have to worry about even more things. Consider case on rv32 when you ask to count (reg:QI s1) and there is a reference to (reg:DI s0). Prior to reload you also have to worry about SUBREGs. You probably need to be using refers_to_regno_p or something similar. jeff
Re: Re: [PATCH] RISC-V: Fix bug reported by PR109535
I tried refers_to_regno_p It can not work for us since it just return true or false whether the "rtx" has the regno. In our situation, we remove "AVL" dependency when it appears once in the "rtx" otherwise, we don't eliminate "AVL" dependency. Would you mind giving me more suggestions? Thanks juzhe.zh...@rivai.ai From: Jeff Law Date: 2023-04-19 09:11 To: juzhe.zh...@rivai.ai; kito.cheng; Richard Biener CC: gcc-patches; palmer Subject: Re: [PATCH] RISC-V: Fix bug reported by PR109535 On 4/18/23 19:04, juzhe.zh...@rivai.ai wrote: > The bug issue reported by google/highway project: > (set(..) > (reg:QI s0) > (reg:DI s0)) > > The "avl" operand rtx = (reg:DI s0) > count_occurrences return 1 however the actual regno occurrences should be 2. > In this case, the VSETVL PASS will eliminate the use of (reg:DI s0) then > file assertion in RTL_SSA. > Instead, we should not eliminate "s0" dependency. So these are not vector hard registers, but GPR hard registers. Meaning you have to worry about even more things. Consider case on rv32 when you ask to count (reg:QI s1) and there is a reference to (reg:DI s0). Prior to reload you also have to worry about SUBREGs. You probably need to be using refers_to_regno_p or something similar. jeff
Re: [PATCH] RISC-V: Fix bug reported by PR109535
On 4/18/23 19:04, juzhe.zh...@rivai.ai wrote: The bug issue reported by google/highway project: (set(..) (reg:QI s0) (reg:DI s0)) The "avl" operand rtx = (reg:DI s0) count_occurrences return 1 however the actual regno occurrences should be 2. In this case, the VSETVL PASS will eliminate the use of (reg:DI s0) then file assertion in RTL_SSA. Instead, we should not eliminate "s0" dependency. So these are not vector hard registers, but GPR hard registers. Meaning you have to worry about even more things. Consider case on rv32 when you ask to count (reg:QI s1) and there is a reference to (reg:DI s0). Prior to reload you also have to worry about SUBREGs. You probably need to be using refers_to_regno_p or something similar. jeff
Re: Re: [PATCH] RISC-V: Fix bug reported by PR109535
The bug issue reported by google/highway project: (set(..) (reg:QI s0) (reg:DI s0)) The "avl" operand rtx = (reg:DI s0) count_occurrences return 1 however the actual regno occurrences should be 2. In this case, the VSETVL PASS will eliminate the use of (reg:DI s0) then file assertion in RTL_SSA. Instead, we should not eliminate "s0" dependency. Thanks juzhe.zh...@rivai.ai From: Jeff Law Date: 2023-04-19 08:56 To: Kito Cheng; juzhe.zhong; Richard Biener CC: gcc-patches; palmer Subject: Re: [PATCH] RISC-V: Fix bug reported by PR109535 On 4/18/23 18:18, Kito Cheng wrote: > Hi Richard, Jeff: > > It's it possible to backport to GCC 13? highway is one of our > important users for RISC-V vector stuff, and it has built in some > distro, so we believe this bug fix is important to backport. I want to see an explanation why count_occurrences isn't doing what you want. jeff
Re: [PATCH] RISC-V: Fix bug reported by PR109535
On 4/18/23 18:18, Kito Cheng wrote: Hi Richard, Jeff: It's it possible to backport to GCC 13? highway is one of our important users for RISC-V vector stuff, and it has built in some distro, so we believe this bug fix is important to backport. I want to see an explanation why count_occurrences isn't doing what you want. jeff
Re: [PATCH] RISC-V: Fix bug reported by PR109535
Hi Richard, Jeff:
It's it possible to backport to GCC 13? highway is one of our
important users for RISC-V vector stuff, and it has built in some
distro, so we believe this bug fix is important to backport.
Thanks
Hi Ju-Zhe:
Thanks for update
On Wed, Apr 19, 2023 at 7:25 AM wrote:
>
> From: Ju-Zhe Zhong
>
> Fix bug reported by google/highway who is using rvv intrinsic:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109535
>
> PR 109535
>
> gcc/ChangeLog:
>
> * config/riscv/riscv-vsetvl.cc (count_regno_occurrences): New
> function.
> (pass_vsetvl::cleanup_insns): Fix bug.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/rvv/base/pr109535.c: New test.
>
> ---
> gcc/config/riscv/riscv-vsetvl.cc | 15 ++-
> .../gcc.target/riscv/rvv/base/pr109535.c | 11 +++
> 2 files changed, 25 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr109535.c
>
> diff --git a/gcc/config/riscv/riscv-vsetvl.cc
> b/gcc/config/riscv/riscv-vsetvl.cc
> index 1b66e3b9eeb..b570b003a1e 100644
> --- a/gcc/config/riscv/riscv-vsetvl.cc
> +++ b/gcc/config/riscv/riscv-vsetvl.cc
> @@ -1592,6 +1592,19 @@ backward_propagate_worthwhile_p (const basic_block
> cfg_bb,
>return true;
> }
>
> +/* Count the number of REGNO in RINSN. */
> +static int
> +count_regno_occurrences (rtx_insn *rinsn, unsigned int regno)
> +{
> + int count = 0;
> + extract_insn (rinsn);
> + for (int i = 0; i < recog_data.n_operands; i++)
> +if (REG_P (recog_data.operand[i])
> + && REGNO (recog_data.operand[i]) == regno)
> + count++;
> + return count;
> +}
> +
> avl_info::avl_info (const avl_info )
> {
>m_value = other.get_value ();
> @@ -3924,7 +3937,7 @@ pass_vsetvl::cleanup_insns (void) const
> if (!has_vl_op (rinsn) || !REG_P (get_vl (rinsn)))
> continue;
> rtx avl = get_vl (rinsn);
> - if (count_occurrences (PATTERN (rinsn), avl, 0) == 1)
> + if (count_regno_occurrences (rinsn, REGNO (avl)) == 1)
> {
> /* Get the list of uses for the new instruction. */
> auto attempt = crtl->ssa->new_change_attempt ();
> diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr109535.c
> b/gcc/testsuite/gcc.target/riscv/rvv/base/pr109535.c
> new file mode 100644
> index 000..7582fe9c392
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr109535.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -march=rv32gcv -mabi=ilp32d" } */
> +
> +#include "riscv_vector.h"
> +
> +void foo(void *in1, void *in2, void *in3, void *out, size_t vl) {
> + vint8m1_t a = __riscv_vle8_v_i8m1(in1, vl);
> + vint8m1_t b = __riscv_vadd_vx_i8m1 (a, vl, vl);
> + __riscv_vse8_v_i8m1(out, b, vl);
> +}
> +
> --
> 2.36.1
>
