[PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[Alex Velenko]

Hi,

This patch permits CSE to add REG_EQUAL notes when single setting a constant
to a register even if REG_EQUAL constant rtx is the same as the set source rtx.
This enables optimizations in later passes looking for REG_EQUAL notes, like
jump2 pass.

For example, in arm testcase pr43920-2.c, CSE previously decided not to put
an "obvious" note on insn 9, as set value was the same as note value.
At the same time, other insns set up as -1 were set up through a register
and did get a note:
(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
        (const_int -1 [0xffffffffffffffff])) /work/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:21 613 {*thumb2_movsi_vfp}
     (nil))

(insn 8 45 50 6 (set (reg:SI 110 [ D.4934 ])
        (reg/v:SI 111 [ startD.4917 ])) /work/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:21 613 {*thumb2_movsi_vfp}
     (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
        (nil)))

(insn 6 49 54 7 (set (reg:SI 110 [ D.4934 ])
        (reg/v:SI 112 [ endD.4918 ])) /work/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:21 613 {*thumb2_movsi_vfp}
     (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
        (nil)))

Jump2 pass, optimizing common code, was looking at notes to reason about
register values and failing to recognize those insns to be equal.

Making CSE to set up REG_EQUAL notes even in "obvious" cases
fixes pr43920-2.c for arm-none-eabi and other targets.

I prefer adding notes in CSE instead of adding additional checks in Jump2
and, if any, other passes, as I think it is more uniform solution and allows
single point fix. Downside is having more notes.

Done full regression run on arm-none-eabi and bootstrapped on x86.

Is patch ok?

gcc/

2015-03-04  Alex Velenko  <Alex.Velenko@arm.com>

	* cse.c (cse_insn): Check to set REG_EQUAL note relaxed.
---
 gcc/cse.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/gcc/cse.c b/gcc/cse.c
index 2a33827..abaf867 100644
--- a/gcc/cse.c
+++ b/gcc/cse.c
@@ -5383,10 +5383,9 @@ cse_insn (rtx_insn *insn)
 	}
 
       /* If this is a single SET, we are setting a register, and we have an
-	 equivalent constant, we want to add a REG_EQUAL note if the constant
-	 is different from the source.  We don't want to do it for a constant
-	 pseudo since verifying that this pseudo hasn't been eliminated is a
-	 pain; moreover such a note won't help anything.
+	 equivalent constant, we want to add a REG_EQUAL note.  We don't want
+	 to do it for a constant pseudo since verifying that this pseudo hasn't
+	 been eliminated is a pain; moreover such a note won't help anything.
 
 	 Avoid a REG_EQUAL note for (CONST (MINUS (LABEL_REF) (LABEL_REF)))
 	 which can be created for a reference to a compile time computable
@@ -5400,8 +5399,7 @@ cse_insn (rtx_insn *insn)
 	  && !(GET_CODE (src_const) == CONST
 	       && GET_CODE (XEXP (src_const, 0)) == MINUS
 	       && GET_CODE (XEXP (XEXP (src_const, 0), 0)) == LABEL_REF
-	       && GET_CODE (XEXP (XEXP (src_const, 0), 1)) == LABEL_REF)
-	  && !rtx_equal_p (src, src_const))
+	       && GET_CODE (XEXP (XEXP (src_const, 0), 1)) == LABEL_REF))
 	{
 	  /* Make sure that the rtx is not shared.  */
 	  src_const = copy_rtx (src_const);
-- 
1.8.1.2

Re: [PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[Segher Boessenkool]

On Wed, Mar 04, 2015 at 11:09:14AM +0000, Alex Velenko wrote:
> I prefer adding notes in CSE instead of adding additional checks in Jump2
> and, if any, other passes, as I think it is more uniform solution and allows
> single point fix. Downside is having more notes.

The other downside is that every other RTL producer will have to add
these notes as well, or passes that look at this info will have to look
at the insns themselves anyway.  Not a good trade-off in my opinion.

Just add a simple helper function to do these checks (isn't there one
already)?


Segher

Re: [PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[Steven Bosscher]

On Wed, Mar 4, 2015 at 12:09 PM, Alex Velenko wrote:
> For example, in arm testcase pr43920-2.c, CSE previously decided not to put
> an "obvious" note on insn 9, as set value was the same as note value.
> At the same time, other insns set up as -1 were set up through a register
> and did get a note:

...which is the point of the REG_EQUAL notes. In insn 8 there is a
REG_EQUAL note to show that the value of r111 is known. In insn 9 the
known value is, well, known from SET_SRC so there is no need for a
REG_EQUAL note. Adding REG_EQUAL notes in such cases is just wasteful.


> (insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
>         (const_int -1 [0xffffffffffffffff])) /work/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:21 613 {*thumb2_movsi_vfp}
>      (nil))
>
> (insn 8 45 50 6 (set (reg:SI 110 [ D.4934 ])
>         (reg/v:SI 111 [ startD.4917 ])) /work/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:21 613 {*thumb2_movsi_vfp}
>      (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
>         (nil)))
>
> (insn 6 49 54 7 (set (reg:SI 110 [ D.4934 ])
>         (reg/v:SI 112 [ endD.4918 ])) /work/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:21 613 {*thumb2_movsi_vfp}
>      (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
>         (nil)))
>
> Jump2 pass, optimizing common code, was looking at notes to reason about
> register values and failing to recognize those insns to be equal.

I suppose you are talking about the head/tail merging code? Can you
please provide a test case for problem preferably filed in Bugzilla)?

Ciao!
Steven

Re: [PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[Jeff Law]

On 03/09/15 03:53, Steven Bosscher wrote:
> On Wed, Mar 4, 2015 at 12:09 PM, Alex Velenko wrote:
>> For example, in arm testcase pr43920-2.c, CSE previously decided not to put
>> an "obvious" note on insn 9, as set value was the same as note value.
>> At the same time, other insns set up as -1 were set up through a register
>> and did get a note:
>
> ...which is the point of the REG_EQUAL notes. In insn 8 there is a
> REG_EQUAL note to show that the value of r111 is known. In insn 9 the
> known value is, well, known from SET_SRC so there is no need for a
> REG_EQUAL note. Adding REG_EQUAL notes in such cases is just wasteful.
RIght.  I'd rather look into why later passes aren't discovering 
whatever equivalences are important rather than adding the redundant notes.

Regardless, I think this is a gcc-6 issue, so I'm not likely to look at 
it in the immediate future.

jeff

Re: [PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[Alex Velenko]

On 09/03/15 17:40, Jeff Law wrote:
> On 03/09/15 03:53, Steven Bosscher wrote:
>> On Wed, Mar 4, 2015 at 12:09 PM, Alex Velenko wrote:
>>> For example, in arm testcase pr43920-2.c, CSE previously decided not to put
>>> an "obvious" note on insn 9, as set value was the same as note value.
>>> At the same time, other insns set up as -1 were set up through a register
>>> and did get a note:
>>
>> ...which is the point of the REG_EQUAL notes. In insn 8 there is a
>> REG_EQUAL note to show that the value of r111 is known. In insn 9 the
>> known value is, well, known from SET_SRC so there is no need for a
>> REG_EQUAL note. Adding REG_EQUAL notes in such cases is just wasteful.
> RIght.  I'd rather look into why later passes aren't discovering
> whatever equivalences are important rather than adding the redundant notes.
>
> Regardless, I think this is a gcc-6 issue, so I'm not likely to look at
> it in the immediate future.
>
> jeff
>
>
Hi Jeff,
I reworked the patch to satisfy your preference.

This patch enables cfgcleanup.c to use const int rtx as REG_EQUAL notes.
For example, this benefits Jump2 to find extra optimisation opportunities.
This patch fixes gcc.target/arm/pr43920-2.c for arm-none-eabi.

Bootstraped on x86, run full regression run on arm-none-eabi and
aarch64-none-elf.

Is this patch ok?

gcc/

2015-03-17  Alex Velenko  <Alex.Velenko@arm.com>

     * cfgcleanup.c (can_replace_by): Use const int rtx of single set as
     REG_EQUAL note.
---
  gcc/cfgcleanup.c | 25 ++++++++++++++++++++-----
  1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
index cee152e..b28a1d3 100644
--- a/gcc/cfgcleanup.c
+++ b/gcc/cfgcleanup.c
@@ -1046,7 +1046,7 @@ equal_different_set_p (rtx p1, rtx s1, rtx p2, rtx s2)
  static enum replace_direction
  can_replace_by (rtx_insn *i1, rtx_insn *i2)
  {
-  rtx s1, s2, d1, d2, src1, src2, note1, note2;
+  rtx s1, s2, d1, d2, src1, src2, note1, note2, cmp_rtx1, cmp_rtx2;
    bool c1, c2;

    /* Check for 2 sets.  */
@@ -1062,12 +1062,29 @@ can_replace_by (rtx_insn *i1, rtx_insn *i2)
          ? rtx_renumbered_equal_p (d1, d2) : rtx_equal_p (d1, d2)))
      return dir_none;

+  src1 = SET_SRC (s1);
+  src2 = SET_SRC (s2);
+
    /* Find identical req_equiv or reg_equal note, which implies that 
the 2 sets
       set dest to the same value.  */
    note1 = find_reg_equal_equiv_note (i1);
    note2 = find_reg_equal_equiv_note (i2);
-  if (!note1 || !note2 || !rtx_equal_p (XEXP (note1, 0), XEXP (note2, 0))
-      || !CONST_INT_P (XEXP (note1, 0)))
+
+  cmp_rtx1 = NULL_RTX;
+  cmp_rtx2 = NULL_RTX;
+
+  if (note1)
+    cmp_rtx1 = XEXP (note1, 0);
+  else if (CONST_INT_P (src1))
+    cmp_rtx1 = src1;
+
+  if (note2)
+    cmp_rtx2 = XEXP (note2, 0);
+  else if (CONST_INT_P (src2))
+    cmp_rtx2 = src2;
+
+  if (!cmp_rtx1 || !cmp_rtx2 || !rtx_equal_p (cmp_rtx1, cmp_rtx2)
+      || !CONST_INT_P (cmp_rtx1))
      return dir_none;

    if (!equal_different_set_p (PATTERN (i1), s1, PATTERN (i2), s2))
@@ -1079,8 +1096,6 @@ can_replace_by (rtx_insn *i1, rtx_insn *i2)
         (set (dest) (reg))
       because we don't know if the reg is live and has the same value 
at the
       location of replacement.  */
-  src1 = SET_SRC (s1);
-  src2 = SET_SRC (s2);
    c1 = CONST_INT_P (src1);
    c2 = CONST_INT_P (src2);
    if (c1 && c2)
-- 1.8.1.2

Re: [PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[Jeff Law]

On 04/10/2015 03:14 AM, Alex Velenko wrote:
> On 09/03/15 17:40, Jeff Law wrote:
>> On 03/09/15 03:53, Steven Bosscher wrote:
>>> On Wed, Mar 4, 2015 at 12:09 PM, Alex Velenko wrote:
>>>> For example, in arm testcase pr43920-2.c, CSE previously decided not
>>>> to put
>>>> an "obvious" note on insn 9, as set value was the same as note value.
>>>> At the same time, other insns set up as -1 were set up through a
>>>> register
>>>> and did get a note:
>>>
>>> ...which is the point of the REG_EQUAL notes. In insn 8 there is a
>>> REG_EQUAL note to show that the value of r111 is known. In insn 9 the
>>> known value is, well, known from SET_SRC so there is no need for a
>>> REG_EQUAL note. Adding REG_EQUAL notes in such cases is just wasteful.
>> RIght.  I'd rather look into why later passes aren't discovering
>> whatever equivalences are important rather than adding the redundant
>> notes.
>>
>> Regardless, I think this is a gcc-6 issue, so I'm not likely to look at
>> it in the immediate future.
>>
>> jeff
>>
>>
> Hi Jeff,
> I reworked the patch to satisfy your preference.
>
> This patch enables cfgcleanup.c to use const int rtx as REG_EQUAL notes.
> For example, this benefits Jump2 to find extra optimisation opportunities.
> This patch fixes gcc.target/arm/pr43920-2.c for arm-none-eabi.
>
> Bootstraped on x86, run full regression run on arm-none-eabi and
> aarch64-none-elf.
>
> Is this patch ok?
>
> gcc/
>
> 2015-03-17  Alex Velenko  <Alex.Velenko@arm.com>
>
>      * cfgcleanup.c (can_replace_by): Use const int rtx of single set as
>      REG_EQUAL note.
Now I finally see this in my queue.  I recalled the discussion around 
whether or not to add the redundant notes, but hadn't had a chance to 
look at the updated patch.

AFAICT, this is redundant with Shiva's patch, right?

jeff

Re: [PATCH] [RTL] Relax CSE check to set REG_EQUAL notes.

[Alex Velenko]

On 24/04/15 02:16, Jeff Law wrote:
> On 04/10/2015 03:14 AM, Alex Velenko wrote:
>> On 09/03/15 17:40, Jeff Law wrote:
>>> On 03/09/15 03:53, Steven Bosscher wrote:
>>>> On Wed, Mar 4, 2015 at 12:09 PM, Alex Velenko wrote:
>>>>> For example, in arm testcase pr43920-2.c, CSE previously decided not
>>>>> to put
>>>>> an "obvious" note on insn 9, as set value was the same as note value.
>>>>> At the same time, other insns set up as -1 were set up through a
>>>>> register
>>>>> and did get a note:
>>>>
>>>> ...which is the point of the REG_EQUAL notes. In insn 8 there is a
>>>> REG_EQUAL note to show that the value of r111 is known. In insn 9 the
>>>> known value is, well, known from SET_SRC so there is no need for a
>>>> REG_EQUAL note. Adding REG_EQUAL notes in such cases is just wasteful.
>>> RIght.  I'd rather look into why later passes aren't discovering
>>> whatever equivalences are important rather than adding the redundant
>>> notes.
>>>
>>> Regardless, I think this is a gcc-6 issue, so I'm not likely to look at
>>> it in the immediate future.
>>>
>>> jeff
>>>
>>>
>> Hi Jeff,
>> I reworked the patch to satisfy your preference.
>>
>> This patch enables cfgcleanup.c to use const int rtx as REG_EQUAL notes.
>> For example, this benefits Jump2 to find extra optimisation opportunities.
>> This patch fixes gcc.target/arm/pr43920-2.c for arm-none-eabi.
>>
>> Bootstraped on x86, run full regression run on arm-none-eabi and
>> aarch64-none-elf.
>>
>> Is this patch ok?
>>
>> gcc/
>>
>> 2015-03-17  Alex Velenko  <Alex.Velenko@arm.com>
>>
>>       * cfgcleanup.c (can_replace_by): Use const int rtx of single set as
>>       REG_EQUAL note.
> Now I finally see this in my queue.  I recalled the discussion around
> whether or not to add the redundant notes, but hadn't had a chance to
> look at the updated patch.
>
> AFAICT, this is redundant with Shiva's patch, right?
>
> jeff
>

Hi Jeff,
Yes, you are correct, this patch is now redundant.
Kind regards,
Alex