diff mbox series

RISC-V: vsetvl: Refine REG_EQUAL equality.

Message ID becd09c9-1353-40ed-a085-be4f938ddf0b@gmail.com
State New
Headers show
Series RISC-V: vsetvl: Refine REG_EQUAL equality. | expand

Commit Message

Robin Dapp Nov. 13, 2023, 8:06 a.m. UTC
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

钟居哲 Nov. 13, 2023, 8:10 a.m. UTC | #1
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 ())
Robin Dapp Nov. 13, 2023, 8:12 a.m. UTC | #2
> 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
钟居哲 Nov. 13, 2023, 8:15 a.m. UTC | #3
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
钟居哲 Nov. 13, 2023, 8:19 a.m. UTC | #4
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
钟居哲 Nov. 13, 2023, 8:25 a.m. UTC | #5
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
Robin Dapp Nov. 13, 2023, 9:25 a.m. UTC | #6
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
钟居哲 Nov. 13, 2023, 9:30 a.m. UTC | #7
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
Robin Dapp Nov. 13, 2023, 9:34 a.m. UTC | #8
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
钟居哲 Nov. 13, 2023, 9:38 a.m. UTC | #9
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
Robin Dapp Nov. 13, 2023, 10:31 a.m. UTC | #10
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;
+}
钟居哲 Nov. 13, 2023, 10:36 a.m. UTC | #11
--- /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;
+}
Robin Dapp Nov. 13, 2023, 1:28 p.m. UTC | #12
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 } } */
钟居哲 Nov. 13, 2023, 1:34 p.m. UTC | #13
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 } } */
Jeff Law Nov. 13, 2023, 1:44 p.m. UTC | #14
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
Robin Dapp Nov. 13, 2023, 2:47 p.m. UTC | #15
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 } } */
Jeff Law Nov. 13, 2023, 6:01 p.m. UTC | #16
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 mbox series

Patch

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 ())