Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Jeff Law]

On 11/13/23 07:47, Robin Dapp wrote:

As per Jeff's remark I'm going to push the attached.

Regards
  Robin

Subject: [PATCH v4] RISC-V: vsetvl: Refine REG_EQUAL equality.

This patch enhances the equality check for REG_EQUAL notes in the vsetvl
pass by using the == operator instead of rtx_equal_p.  With that, in
situations like the following, a5 and a7 are not considered equal
anymore.
One final note.  The register allocator tries to promote REG_EQUAL notes 
to REG_EQUIV notes when it's provably safe.  I don't think that code is 
terribly aggressive and I doubt it'd kick in for the forms shown below.




(insn 62 60 63 4 (set (reg:DI 17 a7 [orig:154 loop_len_54 ] [154])
 (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
 (reg:DI 30 t5 [219]))) 442 {umindi3}
  (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
 (const_int 8 [0x8]))
 (nil)))
(insn 63 62 65 4 (set (reg:DI 15 a5 [orig:175 _103 ] [175])
 (minus:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
 (reg:DI 17 a7 [orig:154 loop_len_54 ] [154]))) 11 {subdi3}
  (nil))
(insn 65 63 66 4 (set (reg:DI 16 a6 [orig:153 loop_len_53 ] [153])
 (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
 (reg:DI 30 t5 [219]))) 442 {umindi3}
  (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
 (const_int 8 [0x8]))
 (nil)))



Jeff

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Robin Dapp]

As per Jeff's remark I'm going to push the attached.

Regards
 Robin

Subject: [PATCH v4] RISC-V: vsetvl: Refine REG_EQUAL equality.

This patch enhances the equality check for REG_EQUAL notes in the vsetvl
pass by using the == operator instead of rtx_equal_p.  With that, in
situations like the following, a5 and a7 are not considered equal
anymore.

(insn 62 60 63 4 (set (reg:DI 17 a7 [orig:154 loop_len_54 ] [154])
(umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(const_int 8 [0x8]))
(nil)))
(insn 63 62 65 4 (set (reg:DI 15 a5 [orig:175 _103 ] [175])
(minus:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 17 a7 [orig:154 loop_len_54 ] [154]))) 11 {subdi3}
 (nil))
(insn 65 63 66 4 (set (reg:DI 16 a6 [orig:153 loop_len_53 ] [153])
(umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(const_int 8 [0x8]))
(nil)))

gcc/ChangeLog:

* config/riscv/riscv-vsetvl.cc (source_equal_p): Use pointer
equality for REG_EQUAL.

gcc/testsuite/ChangeLog:

* gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c: New test.
---
 gcc/config/riscv/riscv-vsetvl.cc  |  6 -
 .../rvv/autovec/partial/multiple_rgroup_zbb.c | 23 +++
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 
gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c

diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 3fa25a6404d..8466b5d019e 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -560,7 +560,11 @@ source_equal_p (insn_info *insn1, insn_info *insn2)
 
   rtx note1 = find_reg_equal_equiv_note (rinsn1);
   rtx note2 = find_reg_equal_equiv_note (rinsn2);
-  if (note1 && note2 && rtx_equal_p (note1, note2))
+  /* We could handle the case of similar-looking REG_EQUALs as well but
+ would need to verify that no insn in between modifies any of the source
+ operands.  */
+  if (note1 && note2 && rtx_equal_p (note1, note2)
+  && REG_NOTE_KIND (note1) == REG_EQUIV)
 return true;
   return false;
 }
diff --git 
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c
new file mode 100644
index 000..15178a2c848
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } *.
+/* { dg-options "-march=rv64gcv_zbb -mabi=lp64d -O2 --param 
riscv-autovec-preference=fixed-vlmax -fno-schedule-insns -fno-schedule-insns2" 
} */
+
+#include 
+
+void __attribute__ ((noipa))
+test (uint16_t *__restrict f, uint32_t *__restrict d, uint64_t *__restrict e,
+  uint16_t x, uint16_t x2, uint16_t x3, uint16_t x4, uint32_t y,
+  uint32_t y2, uint64_t z, int n)
+{
+  for (int i = 0; i < n; ++i)
+{
+  f[i * 4 + 0] = x;
+  f[i * 4 + 1] = x2;
+  f[i * 4 + 2] = x3;
+  f[i * 4 + 3] = x4;
+  d[i * 2 + 0] = y;
+  d[i * 2 + 1] = y2;
+  e[i] = z;
+}
+}
+
+/* { dg-final { scan-assembler-times 
"vsetvli\tzero,\s*\[a-z0-9\]+,\s*e16,\s*m1,\s*ta,\s*ma" 4 } } */
-- 
2.41.0

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Jeff Law]

On 11/13/23 01:15, juzhe.zh...@rivai.ai wrote:

I know the root cause is:

(reg:DI 15 a5 [orig:175 _103 ] [175])

(reg:DI 15 a5 [orig:174 _100 ] [174])


is considered as true on rtx_equal_p.

I think return note1 == note2; will simplify your codes and fix this issue.
NOTEs are not shared (look at how they get chained and it's obvious 
they're not shared).  So you can't use pointer equality, you must use 
rtx_equal_p.


More generally, nodes are not shared unless explicitly documented as 
such in the internals manual, "Sharing" section.


Jeff

Re: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[钟居哲]

I just checked your test. I won't be brittle in the future.
Since it should be 4 vsetvls with e16m1 for SLP AVL/VL toggling.
And also it is no scheduling.  The middle-end MIN_EXPR SLP always produce 4 
AVL/VL toggling
as long as we don't schedule the instructions.

So it won't be problem.

So, LGTM.



juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 21:28
To: juzhe.zh...@rivai.ai; gcc-patches; palmer; kito.cheng; jeffreyalaw
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
On 11/13/23 11:36, juzhe.zh...@rivai.ai wrote:
> --- /dev/null
> +++ 
> b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
> @@ -0,0 +1,19 @@
> +/* { dg-do run { target { riscv_v } } } */
> +/* { dg-additional-options "-march=rv64gcv_zbb --param 
> riscv-autovec-preference=fixed-vlmax" } */
> 
> Could you add compile test (with assembly check) instead of run test ?
 
I found it a bit difficult to create a proper test, hopefully the attached
is not too brittle.
 
My impression is that it would be easier to have such tests if there were
vsetvl statistics of how many vsetvls we merged, fused and for what
reasons etc.
Maybe that's a good learning exercise to get familiar with the pass for
somebody?
 
Regards
Robin
 
Subject: [PATCH v3] RISC-V: vsetvl: Refine REG_EQUAL equality.
 
This patch enhances the equality check for REG_EQUAL notes in the vsetvl
pass by using the == operator instead of rtx_equal_p.  With that, in
situations like the following, a5 and a7 are not considered equal
anymore.
 
(insn 62 60 63 4 (set (reg:DI 17 a7 [orig:154 loop_len_54 ] [154])
(umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(const_int 8 [0x8]))
(nil)))
(insn 63 62 65 4 (set (reg:DI 15 a5 [orig:175 _103 ] [175])
(minus:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 17 a7 [orig:154 loop_len_54 ] [154]))) 11 {subdi3}
 (nil))
(insn 65 63 66 4 (set (reg:DI 16 a6 [orig:153 loop_len_53 ] [153])
(umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(const_int 8 [0x8]))
(nil)))
 
gcc/ChangeLog:
 
* config/riscv/riscv-vsetvl.cc (source_equal_p): Use pointer
equality for REG_EQUAL.
 
gcc/testsuite/ChangeLog:
 
* gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c: New test.
---
gcc/config/riscv/riscv-vsetvl.cc  | 12 +-
.../rvv/autovec/partial/multiple_rgroup_zbb.c | 23 +++
2 files changed, 34 insertions(+), 1 deletion(-)
create mode 100644 
gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c
 
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 3fa25a6404d..63f966f2f3a 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -561,7 +561,17 @@ source_equal_p (insn_info *insn1, insn_info *insn2)
   rtx note1 = find_reg_equal_equiv_note (rinsn1);
   rtx note2 = find_reg_equal_equiv_note (rinsn2);
   if (note1 && note2 && rtx_equal_p (note1, note2))
-return true;
+{
+  /* REG_EQUIVs are invariant at function scope.  */
+  if (REG_NOTE_KIND (note2) == REG_EQUIV)
+ return true;
+
+  /* REG_EQUAL are not so in order to consider them similar the RTX they
+ point to must be identical.  We could also allow "rtx_equal"
+ REG_EQUALs but would need to check if no insn between them modifies
+ any of their sources.  */
+  return note1 == note2;
+}
   return false;
}
diff --git 
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c
new file mode 100644
index 000..15178a2c848
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } *.
+/* { dg-options "-march=rv64gcv_zbb -mabi=lp64d -O2 --param 
riscv-autovec-preference=fixed-vlmax -fno-schedule-insns -fno-schedule-insns2" 
} */
+
+#include 
+
+void __attribute__ ((noipa))
+test (uint16_t *__restrict f, uint32_t *__restrict d, uint64_t *__restrict e,
+  uint16_t x, uint16_t x2, uint16_t x3, uint16_t x4, uint32_t y,
+  uint32_t y2, uint64_t z, int n)
+{
+  for (int i = 0; i < n; ++i)
+{
+  f[i * 4 + 0] = x;
+  f[i * 4 + 1] = x2;
+  f[i * 4 + 2] = x3;
+  f[i * 4 + 3] = x4;
+  d[i * 2 + 0] = y;
+  d[i * 2 + 1] = y2;
+  e[i] = z;
+}
+}
+
+/* { dg-final { scan-assembler-times 
"vsetvli\tzero,\s*\[a-z0-9\]+,\s*e16,\s*m1,\s*ta,\s*ma" 4 } } */
-- 
2.41.0
 
 
 

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Robin Dapp]

On 11/13/23 11:36, juzhe.zh...@rivai.ai wrote:
> --- /dev/null
> +++ 
> b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
> @@ -0,0 +1,19 @@
> +/* { dg-do run { target { riscv_v } } } */
> +/* { dg-additional-options "-march=rv64gcv_zbb --param 
> riscv-autovec-preference=fixed-vlmax" } */
> 
> Could you add compile test (with assembly check) instead of run test ?

I found it a bit difficult to create a proper test, hopefully the attached
is not too brittle.

My impression is that it would be easier to have such tests if there were
vsetvl statistics of how many vsetvls we merged, fused and for what
reasons etc.
Maybe that's a good learning exercise to get familiar with the pass for
somebody?

Regards
 Robin

Subject: [PATCH v3] RISC-V: vsetvl: Refine REG_EQUAL equality.

This patch enhances the equality check for REG_EQUAL notes in the vsetvl
pass by using the == operator instead of rtx_equal_p.  With that, in
situations like the following, a5 and a7 are not considered equal
anymore.

(insn 62 60 63 4 (set (reg:DI 17 a7 [orig:154 loop_len_54 ] [154])
(umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(const_int 8 [0x8]))
(nil)))
(insn 63 62 65 4 (set (reg:DI 15 a5 [orig:175 _103 ] [175])
(minus:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 17 a7 [orig:154 loop_len_54 ] [154]))) 11 {subdi3}
 (nil))
(insn 65 63 66 4 (set (reg:DI 16 a6 [orig:153 loop_len_53 ] [153])
(umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(const_int 8 [0x8]))
(nil)))

gcc/ChangeLog:

* config/riscv/riscv-vsetvl.cc (source_equal_p): Use pointer
equality for REG_EQUAL.

gcc/testsuite/ChangeLog:

* gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c: New test.
---
 gcc/config/riscv/riscv-vsetvl.cc  | 12 +-
 .../rvv/autovec/partial/multiple_rgroup_zbb.c | 23 +++
 2 files changed, 34 insertions(+), 1 deletion(-)
 create mode 100644 
gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c

diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 3fa25a6404d..63f966f2f3a 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -561,7 +561,17 @@ source_equal_p (insn_info *insn1, insn_info *insn2)
   rtx note1 = find_reg_equal_equiv_note (rinsn1);
   rtx note2 = find_reg_equal_equiv_note (rinsn2);
   if (note1 && note2 && rtx_equal_p (note1, note2))
-return true;
+{
+  /* REG_EQUIVs are invariant at function scope.  */
+  if (REG_NOTE_KIND (note2) == REG_EQUIV)
+   return true;
+
+  /* REG_EQUAL are not so in order to consider them similar the RTX they
+point to must be identical.  We could also allow "rtx_equal"
+REG_EQUALs but would need to check if no insn between them modifies
+any of their sources.  */
+  return note1 == note2;
+}
   return false;
 }
 
diff --git 
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c
new file mode 100644
index 000..15178a2c848
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb.c
@@ -0,0 +1,23 @@
+/* { dg-do compile } *.
+/* { dg-options "-march=rv64gcv_zbb -mabi=lp64d -O2 --param 
riscv-autovec-preference=fixed-vlmax -fno-schedule-insns -fno-schedule-insns2" 
} */
+
+#include 
+
+void __attribute__ ((noipa))
+test (uint16_t *__restrict f, uint32_t *__restrict d, uint64_t *__restrict e,
+  uint16_t x, uint16_t x2, uint16_t x3, uint16_t x4, uint32_t y,
+  uint32_t y2, uint64_t z, int n)
+{
+  for (int i = 0; i < n; ++i)
+{
+  f[i * 4 + 0] = x;
+  f[i * 4 + 1] = x2;
+  f[i * 4 + 2] = x3;
+  f[i * 4 + 3] = x4;
+  d[i * 2 + 0] = y;
+  d[i * 2 + 1] = y2;
+  e[i] = z;
+}
+}
+
+/* { dg-final { scan-assembler-times 
"vsetvli\tzero,\s*\[a-z0-9\]+,\s*e16,\s*m1,\s*ta,\s*ma" 4 } } */
-- 
2.41.0

Re: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[juzhe.zh...@rivai.ai]

--- /dev/null
+++ 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
@@ -0,0 +1,19 @@
+/* { dg-do run { target { riscv_v } } } */
+/* { dg-additional-options "-march=rv64gcv_zbb --param 
riscv-autovec-preference=fixed-vlmax" } */

Could you add compile test (with assembly check) instead of run test ?

If I don't build toolchain with "zbb" then we can't test such issue (VSETVL 
BUG).
I may cause regression again if I change VSETVL pass in the future.



juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 18:31
To: juzhe.zh...@rivai.ai; gcc-patches; palmer; kito.cheng; jeffreyalaw
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
On 11/13/23 10:38, juzhe.zh...@rivai.ai wrote:
> For @code{REG_EQUIV}, the register is equivalent to @var{op} throughout
> the entire function, and could validly be replaced in all its
> occurrences by @var{op}.  (``Validly'' here refers to the data flow of
> the program; simple replacement may make some insns invalid.)  For
> example, when a constant is loaded into a register that is never
> assigned any other value, this kind of note is used.
> 
> I think REG_QEUIV is what I want. So I think you can test it to see if there 
> is regression on current tests.
 
Let's keep the REG_EQUAL optimization for later in case we find
a case that triggers it.
 
Regards
Robin
 
Subject: [PATCH v2] RISC-V: vsetvl: Refine REG_EQUAL equality.
 
This patch enhances the equality check for REG_EQUAL notes in the vsetvl
pass by using the == operator instead of rtx_equal_p.  With that, in
situations like the following, a5 and a7 are not considered equal
anymore.
 
(insn 62 60 63 4 (set (reg:DI 17 a7 [orig:154 loop_len_54 ] [154])
(umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(const_int 8 [0x8]))
(nil)))
(insn 63 62 65 4 (set (reg:DI 15 a5 [orig:175 _103 ] [175])
(minus:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 17 a7 [orig:154 loop_len_54 ] [154]))) 11 {subdi3}
 (nil))
(insn 65 63 66 4 (set (reg:DI 16 a6 [orig:153 loop_len_53 ] [153])
(umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(const_int 8 [0x8]))
(nil)))
 
gcc/ChangeLog:
 
* config/riscv/riscv-vsetvl.cc (source_equal_p): Use pointer
equality for REG_EQUAL.
 
gcc/testsuite/ChangeLog:
 
* gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c: New test.
---
gcc/config/riscv/riscv-vsetvl.cc  | 12 +++-
.../partial/multiple_rgroup_zbb_run-2.c   | 19 +++
2 files changed, 30 insertions(+), 1 deletion(-)
create mode 100644 
gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
 
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 3fa25a6404d..0eea33dd0e1 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -561,7 +561,17 @@ source_equal_p (insn_info *insn1, insn_info *insn2)
   rtx note1 = find_reg_equal_equiv_note (rinsn1);
   rtx note2 = find_reg_equal_equiv_note (rinsn2);
   if (note1 && note2 && rtx_equal_p (note1, note2))
-return true;
+{
+  /* REG_EQUIVs are invariant at function scope.  */
+  if (REG_NOTE_KIND (note2) == REG_EQUIV)
+ return true;
+
+  /* REG_EQUAL are not so in order to consider them similar the RTX they
+ point to must be identical.  We could also allow "rtx_equal"
+ REG_EQUALs but would need to check if no insn between them modifies
+ any of their sources.  */
+  return note1 == note2;
+}
   return false;
}
diff --git 
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
new file mode 100644
index 000..aeb337dc7ee
--- /dev/null
+++ 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
@@ -0,0 +1,19 @@
+/* { dg-do run { target { riscv_v } } } */
+/* { dg-additional-options "-march=rv64gcv_zbb --param 
riscv-autovec-preference=fixed-vlmax" } */
+
+#include "multiple_rgroup-2.c"
+
+int main (void)
+{
+  TEST_ALL (run_1)
+  TEST_ALL (run_2)
+  TEST_ALL (run_3)
+  TEST_ALL (run_4)
+  TEST_ALL (run_5)
+  TEST_ALL (run_6)
+  TEST_ALL (run_7)
+  TEST_ALL (run_8)
+  TEST_ALL (run_9)
+  TEST_ALL (run_10)
+  return 0;
+}
-- 
2.41.0
 
 

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Robin Dapp]

On 11/13/23 10:38, juzhe.zh...@rivai.ai wrote:
> For @code{REG_EQUIV}, the register is equivalent to @var{op} throughout
> the entire function, and could validly be replaced in all its
> occurrences by @var{op}.  (``Validly'' here refers to the data flow of
> the program; simple replacement may make some insns invalid.)  For
> example, when a constant is loaded into a register that is never
> assigned any other value, this kind of note is used.
> 
> I think REG_QEUIV is what I want. So I think you can test it to see if there 
> is regression on current tests.

Let's keep the REG_EQUAL optimization for later in case we find
a case that triggers it.

Regards
 Robin

Subject: [PATCH v2] RISC-V: vsetvl: Refine REG_EQUAL equality.

This patch enhances the equality check for REG_EQUAL notes in the vsetvl
pass by using the == operator instead of rtx_equal_p.  With that, in
situations like the following, a5 and a7 are not considered equal
anymore.

(insn 62 60 63 4 (set (reg:DI 17 a7 [orig:154 loop_len_54 ] [154])
(umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(const_int 8 [0x8]))
(nil)))
(insn 63 62 65 4 (set (reg:DI 15 a5 [orig:175 _103 ] [175])
(minus:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 17 a7 [orig:154 loop_len_54 ] [154]))) 11 {subdi3}
 (nil))
(insn 65 63 66 4 (set (reg:DI 16 a6 [orig:153 loop_len_53 ] [153])
(umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(const_int 8 [0x8]))
(nil)))

gcc/ChangeLog:

* config/riscv/riscv-vsetvl.cc (source_equal_p): Use pointer
equality for REG_EQUAL.

gcc/testsuite/ChangeLog:

* gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c: New 
test.
---
 gcc/config/riscv/riscv-vsetvl.cc  | 12 +++-
 .../partial/multiple_rgroup_zbb_run-2.c   | 19 +++
 2 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 
gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c

diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 3fa25a6404d..0eea33dd0e1 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -561,7 +561,17 @@ source_equal_p (insn_info *insn1, insn_info *insn2)
   rtx note1 = find_reg_equal_equiv_note (rinsn1);
   rtx note2 = find_reg_equal_equiv_note (rinsn2);
   if (note1 && note2 && rtx_equal_p (note1, note2))
-return true;
+{
+  /* REG_EQUIVs are invariant at function scope.  */
+  if (REG_NOTE_KIND (note2) == REG_EQUIV)
+   return true;
+
+  /* REG_EQUAL are not so in order to consider them similar the RTX they
+point to must be identical.  We could also allow "rtx_equal"
+REG_EQUALs but would need to check if no insn between them modifies
+any of their sources.  */
+  return note1 == note2;
+}
   return false;
 }
 
diff --git 
a/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
new file mode 100644
index 000..aeb337dc7ee
--- /dev/null
+++ 
b/gcc/testsuite/gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_zbb_run-2.c
@@ -0,0 +1,19 @@
+/* { dg-do run { target { riscv_v } } } */
+/* { dg-additional-options "-march=rv64gcv_zbb --param 
riscv-autovec-preference=fixed-vlmax" } */
+
+#include "multiple_rgroup-2.c"
+
+int main (void)
+{
+  TEST_ALL (run_1)
+  TEST_ALL (run_2)
+  TEST_ALL (run_3)
+  TEST_ALL (run_4)
+  TEST_ALL (run_5)
+  TEST_ALL (run_6)
+  TEST_ALL (run_7)
+  TEST_ALL (run_8)
+  TEST_ALL (run_9)
+  TEST_ALL (run_10)
+  return 0;
+}
-- 
2.41.0

Re: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[juzhe.zh...@rivai.ai]

For @code{REG_EQUIV}, the register is equivalent to @var{op} throughout
the entire function, and could validly be replaced in all its
occurrences by @var{op}.  (``Validly'' here refers to the data flow of
the program; simple replacement may make some insns invalid.)  For
example, when a constant is loaded into a register that is never
assigned any other value, this kind of note is used.

I think REG_QEUIV is what I want. So I think you can test it to see if there is 
regression on current tests.


juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 17:34
To: juzhe.zh...@rivai.ai; gcc-patches; palmer; kito.cheng; jeffreyalaw
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
On 11/13/23 10:30, juzhe.zh...@rivai.ai wrote:
> I just checked definition of REG_EQUAL and REG_EQUIV.
> 
> As you said, REG_EQUIV is more reasonable. Agree with use rtx_equal_p on 
> REG_EQUIV and skip REG_EQUAL.
> Could you check whether it does fix your issues ?
 
Yes it would fix the issues.  I just figured we could work
a bit harder and also catch cases where two "different"
REG_EQUALS would still be the same.  But I'm not sure whether
such cases exist at all (leaning towards no...).
 
Going to post v2 after the tests ran.
 
Regards
Robin
 
 

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Robin Dapp]

On 11/13/23 10:30, juzhe.zh...@rivai.ai wrote:
> I just checked definition of REG_EQUAL and REG_EQUIV.
> 
> As you said, REG_EQUIV is more reasonable. Agree with use rtx_equal_p on 
> REG_EQUIV and skip REG_EQUAL.
> Could you check whether it does fix your issues ?

Yes it would fix the issues.  I just figured we could work
a bit harder and also catch cases where two "different"
REG_EQUALS would still be the same.  But I'm not sure whether
such cases exist at all (leaning towards no...).

Going to post v2 after the tests ran.

Regards
 Robin

Re: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[juzhe.zh...@rivai.ai]

I just checked definition of REG_EQUAL and REG_EQUIV.

As you said, REG_EQUIV is more reasonable. Agree with use rtx_equal_p on 
REG_EQUIV and skip REG_EQUAL.
Could you check whether it does fix your issues ?



juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 17:25
To: juzhe.zh...@rivai.ai; gcc-patches; palmer; kito.cheng; jeffreyalaw
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
On 11/13/23 09:25, juzhe.zh...@rivai.ai wrote:
> Also, like kito previous remind me:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635326.html 
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635326.html> 
> 
> I think you should add a dedicated test which with specifying 
> -march=rv64gcv_zbb,
> then scan-assembler-check  the correct vsetvl.
> 
> So that we can allow people like me be able to avoid regression of such issue 
> even if I didn't build toolchain with "zbb".
 
Yes, makes sense.  Added.
 
Regarding the use of == instead of rtx_equal_p.  Hmm, I'm not sure
if pointer equality works.  Do we really re-use the exact rtx note
for a different insn?
 
When I use note1 == note2 (instead of equal_p) there are regressions:
 
FAIL: gcc.target/riscv/rvv/autovec/vls/dup-1.c -O3 -ftree-vectorize --param 
riscv-autovec-preference=scalable  check-function-bodies foo10
FAIL: gcc.target/riscv/rvv/autovec/vls/dup-2.c -O3 -ftree-vectorize --param 
riscv-autovec-preference=scalable  check-function-bodies foo10
FAIL: gcc.target/riscv/rvv/autovec/vls/dup-3.c -O3 -ftree-vectorize --param 
riscv-autovec-preference=scalable  check-function-bodies foo10
FAIL: gcc.target/riscv/rvv/vsetvl/avl_single-102.c   -O2   scan-assembler-times 
vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/avl_single-102.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/avl_single-102.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times vsetvli 1
 
So what we could do instead is rtx_equal_p for REG_EQUIV and skip
REG_EQUAL altogether?
 
Regards
Robin
 

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Robin Dapp]

On 11/13/23 09:25, juzhe.zh...@rivai.ai wrote:
> Also, like kito previous remind me:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635326.html 
>  
> 
> I think you should add a dedicated test which with specifying 
> -march=rv64gcv_zbb,
> then scan-assembler-check  the correct vsetvl.
> 
> So that we can allow people like me be able to avoid regression of such issue 
> even if I didn't build toolchain with "zbb".

Yes, makes sense.  Added.

Regarding the use of == instead of rtx_equal_p.  Hmm, I'm not sure
if pointer equality works.  Do we really re-use the exact rtx note
for a different insn?

When I use note1 == note2 (instead of equal_p) there are regressions:

FAIL: gcc.target/riscv/rvv/autovec/vls/dup-1.c -O3 -ftree-vectorize --param 
riscv-autovec-preference=scalable  check-function-bodies foo10
FAIL: gcc.target/riscv/rvv/autovec/vls/dup-2.c -O3 -ftree-vectorize --param 
riscv-autovec-preference=scalable  check-function-bodies foo10
FAIL: gcc.target/riscv/rvv/autovec/vls/dup-3.c -O3 -ftree-vectorize --param 
riscv-autovec-preference=scalable  check-function-bodies foo10
FAIL: gcc.target/riscv/rvv/vsetvl/avl_single-102.c   -O2   scan-assembler-times 
vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/avl_single-102.c   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none   scan-assembler-times vsetvli 1
FAIL: gcc.target/riscv/rvv/vsetvl/avl_single-102.c   -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects   scan-assembler-times vsetvli 1

So what we could do instead is rtx_equal_p for REG_EQUIV and skip
REG_EQUAL altogether?

Regards
 Robin

Re: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[juzhe.zh...@rivai.ai]

Also, like kito previous remind me:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635326.html 

I think you should add a dedicated test which with specifying 
-march=rv64gcv_zbb,
then scan-assembler-check  the correct vsetvl.

So that we can allow people like me be able to avoid regression of such issue 
even if I didn't build toolchain with "zbb".



juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 16:12
To: juzhe.zh...@rivai.ai; gcc-patches; palmer; kito.cheng; jeffreyalaw
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
> Does this patch fixes exposed bugs in current tests?
> Or could you add test for it ?
 
Ah, yes forgot to mention.  This fixes several tests when
testing with -march=rv64gcv_zbb.
 
Regards
Robin
 

Re: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[juzhe.zh...@rivai.ai]

Sorry. It should be return note1 && note2 && note1 == note2;



juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 16:12
To: juzhe.zh...@rivai.ai; gcc-patches; palmer; kito.cheng; jeffreyalaw
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
> Does this patch fixes exposed bugs in current tests?
> Or could you add test for it ?
 
Ah, yes forgot to mention.  This fixes several tests when
testing with -march=rv64gcv_zbb.
 
Regards
Robin
 

Re: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[juzhe.zh...@rivai.ai]

I know the root cause is:

(reg:DI 15 a5 [orig:175 _103 ] [175])(reg:DI 15 a5 [orig:174 _100 ] [174])

is considered as true on rtx_equal_p.

I think return note1 == note2; will simplify your codes and fix this issue.



juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 16:12
To: juzhe.zh...@rivai.ai; gcc-patches; palmer; kito.cheng; jeffreyalaw
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
> Does this patch fixes exposed bugs in current tests?
> Or could you add test for it ?
 
Ah, yes forgot to mention.  This fixes several tests when
testing with -march=rv64gcv_zbb.
 
Regards
Robin
 

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[Robin Dapp]

> Does this patch fixes exposed bugs in current tests?
> Or could you add test for it ?

Ah, yes forgot to mention.  This fixes several tests when
testing with -march=rv64gcv_zbb.

Regards
 Robin

Re: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.

[juzhe.zh...@rivai.ai]

Does this patch fixes exposed bugs in current tests?
Or could you add test for it ?



juzhe.zh...@rivai.ai
 
From: Robin Dapp
Date: 2023-11-13 16:06
To: gcc-patches; palmer; Kito Cheng; jeffreyalaw; juzhe.zh...@rivai.ai
CC: rdapp.gcc
Subject: [PATCH] RISC-V: vsetvl: Refine REG_EQUAL equality.
Hi,
 
this patch enhances the equality check for REG_EQUAL notes in the vsetvl
pass.  Currently, we assume that two such notes describe the same value
when they have the same rtx representation.  This is not true when
either of the note's source operands is modified by an insn between the
two notes.
 
Suppose:
 
(insn 62 60 63 4 (set (reg:DI 17 a7 [orig:154 loop_len_54 ] [154])
(umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(const_int 8 [0x8]))
(nil)))
(insn 63 62 65 4 (set (reg:DI 15 a5 [orig:175 _103 ] [175])
(minus:DI (reg:DI 15 a5 [orig:174 _100 ] [174])
(reg:DI 17 a7 [orig:154 loop_len_54 ] [154]))) 11 {subdi3}
 (nil))
(insn 65 63 66 4 (set (reg:DI 16 a6 [orig:153 loop_len_53 ] [153])
(umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(reg:DI 30 t5 [219]))) 442 {umindi3}
 (expr_list:REG_EQUAL (umin:DI (reg:DI 15 a5 [orig:175 _103 ] [175])
(const_int 8 [0x8]))
(nil)))
 
where insn 63 overwrites a5 and insn 65's REG_EQUAL note that refers to
a5 describes a different value than insn 62's REG_EQUAL note.
 
In order to catch this situation this patch has source_equal_p check
every instruction between two notes for modification of any
participating register.
 
Regards
Robin
 
 
gcc/ChangeLog:
 
* config/riscv/riscv-vsetvl.cc (modify_reg_between_p): Move.
(source_equal_p): Check if source registers were modified in
between.
---
gcc/config/riscv/riscv-vsetvl.cc | 62 
1 file changed, 47 insertions(+), 15 deletions(-)
 
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 3fa25a6404d..34bf7498103 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -85,6 +85,7 @@ along with GCC; see the file COPYING3.  If not see
#include "predict.h"
#include "profile-count.h"
#include "gcse.h"
+#include "rtl-iter.h"
using namespace rtl_ssa;
using namespace riscv_vector;
@@ -548,6 +549,21 @@ get_all_sets (set_info *set, bool /* get_real_inst */ 
real_p,
   return hash_set ();
}
+static bool
+modify_reg_between_p (insn_info *prev_insn, insn_info *curr_insn,
+   unsigned regno)
+{
+  gcc_assert (prev_insn->compare_with (curr_insn) < 0);
+  for (insn_info *i = curr_insn->prev_nondebug_insn (); i != prev_insn;
+   i = i->prev_nondebug_insn ())
+{
+  // no def of regno
+  if (find_access (i->defs (), regno))
+ return true;
+}
+  return false;
+}
+
static bool
source_equal_p (insn_info *insn1, insn_info *insn2)
{
@@ -561,7 +577,37 @@ source_equal_p (insn_info *insn1, insn_info *insn2)
   rtx note1 = find_reg_equal_equiv_note (rinsn1);
   rtx note2 = find_reg_equal_equiv_note (rinsn2);
   if (note1 && note2 && rtx_equal_p (note1, note2))
-return true;
+{
+  /* REG_EQUIVs are globally.  */
+  if (REG_NOTE_KIND (note2) == REG_EQUIV)
+ return true;
+
+  /* If both insns are the same, the notes are definitely equivalent.  */
+  if (insn2->compare_with (insn1) == 0)
+ return true;
+
+  /* Canonicalize order so insn1 is always before insn2 for the following
+ check.  */
+  if (insn2->compare_with (insn1) < 0)
+ std::swap (insn1, insn2);
+
+  /* If two REG_EQUAL notes are similar the value they calculate can still
+ be different.  The value is only identical if none of the sources have
+ been modified in between.  */
+  subrtx_iterator::array_type array;
+  FOR_EACH_SUBRTX (iter, array, note2, NONCONST)
+ {
+   if (!*iter)
+ continue;
+
+   if (!REG_P (*iter))
+ continue;
+
+   if (modify_reg_between_p (insn1, insn2, REGNO (*iter)))
+ return false;
+ }
+  return true;
+}
   return false;
}
@@ -1439,20 +1485,6 @@ private:
   && find_access (i->defs (), REGNO (info.get_avl ()));
   }
-  inline bool modify_reg_between_p (insn_info *prev_insn, insn_info *curr_insn,
- unsigned regno)
-  {
-gcc_assert (prev_insn->compare_with (curr_insn) < 0);
-for (insn_info *i = curr_insn->prev_nondebug_insn (); i != prev_insn;
- i = i->prev_nondebug_insn ())
-  {
- // no def of regno
- if (find_access (i->defs (), regno))
-   return true;
-  }
-return false;
-  }
-
   inline bool reg_avl_equal_p (const vsetvl_info , const vsetvl_info 
)
   {
 if (!prev.has_nonvlmax_reg_avl () || !next.has_nonvlmax_reg_avl ())
-- 
2.41.0