diff mbox series

RISC-V: Fix bug reported by PR109535

Message ID 20230418232515.95891-1-juzhe.zhong@rivai.ai
State Accepted
Headers
Series RISC-V: Fix bug reported by PR109535 |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

juzhe.zhong@rivai.ai April 18, 2023, 11:25 p.m. UTC 
  From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>

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

Comments

Kito Cheng April 19, 2023, 12:18 a.m. UTC | #1 
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 <juzhe.zhong@rivai.ai> wrote:
>
> From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai>
>
> 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 &other)
>  {
>    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 00000000000..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
>
Jeff Law April 19, 2023, 12:56 a.m. UTC | #2 
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
juzhe.zhong@rivai.ai April 19, 2023, 1:04 a.m. UTC | #3 
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.zhong@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
Jeff Law April 19, 2023, 1:11 a.m. UTC | #4 
On 4/18/23 19:04, juzhe.zhong@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
juzhe.zhong@rivai.ai April 19, 2023, 1:29 a.m. UTC | #5 
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.zhong@rivai.ai
 
From: Jeff Law
Date: 2023-04-19 09:11
To: juzhe.zhong@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.zhong@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
juzhe.zhong@rivai.ai April 19, 2023, 1:34 a.m. UTC | #6 
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.zhong@rivai.ai
 
From: Jeff Law
Date: 2023-04-19 09:11
To: juzhe.zhong@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.zhong@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
Jeff Law April 19, 2023, 5:43 a.m. UTC | #7 
On 4/18/23 19:29, juzhe.zhong@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
juzhe.zhong@rivai.ai April 19, 2023, 10:45 a.m. UTC | #8 
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.zhong@rivai.ai
 
From: Jeff Law
Date: 2023-04-19 13:43
To: juzhe.zhong@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.zhong@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
diff mbox series

Patch

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 &other)
 {
   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 00000000000..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);
+}
+