RISC-V: vsetvl: Refine REG_EQUAL equality.
Checks
Commit Message
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(-)
Comments
> 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 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;
+}
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
@@ -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 ())