| Message ID | becd09c9-1353-40ed-a085-be4f938ddf0b@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: vsetvl: Refine REG_EQUAL equality. | expand |
Does this patch fixes exposed bugs in current tests?
Or could you add test for it ?
juzhe.zhong@rivai.ai
From: Robin Dapp
Date: 2023-11-13 16:06
To: gcc-patches; palmer; Kito Cheng; jeffreyalaw; juzhe.zhong@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<set_info *> ();
}
+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 &prev, const vsetvl_info &next)
{
if (!prev.has_nonvlmax_reg_avl () || !next.has_nonvlmax_reg_avl ())
> 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
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.zhong@rivai.ai From: Robin Dapp Date: 2023-11-13 16:12 To: juzhe.zhong@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
Sorry. It should be return note1 && note2 && note1 == note2; juzhe.zhong@rivai.ai From: Robin Dapp Date: 2023-11-13 16:12 To: juzhe.zhong@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
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.zhong@rivai.ai From: Robin Dapp Date: 2023-11-13 16:12 To: juzhe.zhong@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
On 11/13/23 09:25, juzhe.zhong@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
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.zhong@rivai.ai From: Robin Dapp Date: 2023-11-13 17:25 To: juzhe.zhong@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.zhong@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
On 11/13/23 10:30, juzhe.zhong@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
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.zhong@rivai.ai
From: Robin Dapp
Date: 2023-11-13 17:34
To: juzhe.zhong@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.zhong@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
On 11/13/23 10:38, juzhe.zhong@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 00000000000..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; +}
--- /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.zhong@rivai.ai
From: Robin Dapp
Date: 2023-11-13 18:31
To: juzhe.zhong@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.zhong@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 00000000000..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;
+}
On 11/13/23 11:36, juzhe.zhong@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 00000000000..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 <stdint-gcc.h> + +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 } } */
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.zhong@rivai.ai From: Robin Dapp Date: 2023-11-13 21:28 To: juzhe.zhong@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.zhong@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 00000000000..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 <stdint-gcc.h> + +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 } } */
On 11/13/23 01:15, juzhe.zhong@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
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 00000000000..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 <stdint-gcc.h>
+
+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 } } */
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
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<set_info *> (); } +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 &prev, const vsetvl_info &next) { if (!prev.has_nonvlmax_reg_avl () || !next.has_nonvlmax_reg_avl ())
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(-)