[PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

* [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
@ 2014-09-22 14:40 Felix Yang
  2014-09-22 18:40 ` Jeff Law
  0 siblings, 1 reply; 27+ messages in thread
From: Felix Yang @ 2014-09-22 14:40 UTC (permalink / raw)
  To: GCC Patches, vmakarov

Hi,

    I find that update_equiv_regs in ira.c sets the wrong EQUIV
reg-note for pseudo with more than one definiton in certain situation.
    Here is a simplified RTL snippet after this function finishs handling:

 (insn 77 37 78 2 (set (reg:SI 171)
         (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
      (expr_list:REG_EQUAL (const_int 0 [0])
         (nil)))

......

(insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
         (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
      (expr_list:REG_DEAD (reg:SI 171)
         (nil)))
(insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
         (reg:SI 163)) 52 {movsi_internal_dsp}
      (expr_list:REG_DEAD (reg:SI 163)
         (expr_list:REG_DEAD (reg/f:SI 162)
             (nil))))
(insn 54 53 14 2 (set (reg:SI 171)
         (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
A32])) ticket151.c:49 52 {movsi_internal_dsp}
      (expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
[flags 0x2]) [4  S4 A32])
         (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
[flags 0x2]) [4  S4 A32])
             (nil))))


    The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
in insn 77 with a differerent value.
    This may causes reload replacing pseudo 171 with mem/u/c:SI
(symbol_ref/u:SI ("*.LC8"), which is wrong.
    A proposed patch for this issue, please comment:

 Index: gcc/ira.c
===================================================================
--- gcc/ira.c    (revision 215460)
+++ gcc/ira.c    (working copy)
@@ -3477,18 +3477,26 @@ update_equiv_regs (void)
           no_equiv (dest, set, NULL);
           continue;
         }
+
       /* Record this insn as initializing this register.  */
       reg_equiv[regno].init_insns
         = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);

       /* If this register is known to be equal to a constant, record that
          it is always equivalent to the constant.  */
-      if (DF_REG_DEF_COUNT (regno) == 1
-          && note && ! rtx_varies_p (XEXP (note, 0), 0))
+      if (note && ! rtx_varies_p (XEXP (note, 0), 0))
         {
-          rtx note_value = XEXP (note, 0);
-          remove_note (insn, note);
-          set_unique_reg_note (insn, REG_EQUIV, note_value);
+          if (DF_REG_DEF_COUNT (regno) == 1)
+        {
+          rtx note_value = XEXP (note, 0);
+          remove_note (insn, note);
+          set_unique_reg_note (insn, REG_EQUIV, note_value);
+        }
+          else
+        {
+              no_equiv (dest, set, NULL);
+              continue;
+        }
         }

       /* If this insn introduces a "constant" register, decrease the priority


Cheers,
Felix

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-22 14:40 [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition Felix Yang
@ 2014-09-22 18:40 ` Jeff Law
  2014-09-23  2:48   ` Yangfei (Felix)
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Law @ 2014-09-22 18:40 UTC (permalink / raw)
  To: Felix Yang, GCC Patches, vmakarov

On 09/22/14 08:40, Felix Yang wrote:
> Hi,
>
>      I find that update_equiv_regs in ira.c sets the wrong EQUIV
> reg-note for pseudo with more than one definiton in certain situation.
>      Here is a simplified RTL snippet after this function finishs handling:
>
>   (insn 77 37 78 2 (set (reg:SI 171)
>           (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
>        (expr_list:REG_EQUAL (const_int 0 [0])
>           (nil)))
>
> ......
>
> (insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
>           (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
>        (expr_list:REG_DEAD (reg:SI 171)
>           (nil)))
> (insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
>           (reg:SI 163)) 52 {movsi_internal_dsp}
>        (expr_list:REG_DEAD (reg:SI 163)
>           (expr_list:REG_DEAD (reg/f:SI 162)
>               (nil))))
> (insn 54 53 14 2 (set (reg:SI 171)
>           (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
> A32])) ticket151.c:49 52 {movsi_internal_dsp}
>        (expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
> [flags 0x2]) [4  S4 A32])
>           (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
> [flags 0x2]) [4  S4 A32])
>               (nil))))
>
>
>      The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
> in insn 77 with a differerent value.
>      This may causes reload replacing pseudo 171 with mem/u/c:SI
> (symbol_ref/u:SI ("*.LC8"), which is wrong.
>      A proposed patch for this issue, please comment:
Is it possible it's the conditional just above this one that is incorrect?

          if (DF_REG_DEF_COUNT (regno) != 1
               && (! note
                   || rtx_varies_p (XEXP (note, 0), 0)
                   || (reg_equiv[regno].replacement
                       && ! rtx_equal_p (XEXP (note, 0),
                                         reg_equiv[regno].replacement))))
             {
               no_equiv (dest, set, NULL);
               continue;
             }


ISTM the only time a multiply-set register can have a valid REG_EQUIV 
note is if each and every assignment to that pseudo has the same RHS.

Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-22 18:40 ` Jeff Law
@ 2014-09-23  2:48   ` Yangfei (Felix)
  2014-09-23 10:46     ` Felix Yang
  0 siblings, 1 reply; 27+ messages in thread
From: Yangfei (Felix) @ 2014-09-23  2:48 UTC (permalink / raw)
  To: Jeff Law, Felix Yang, GCC Patches, vmakarov

> On 09/22/14 08:40, Felix Yang wrote:
> > Hi,
> >
> >      I find that update_equiv_regs in ira.c sets the wrong EQUIV
> > reg-note for pseudo with more than one definiton in certain situation.
> >      Here is a simplified RTL snippet after this function finishs handling:
> >
> >   (insn 77 37 78 2 (set (reg:SI 171)
> >           (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
> >        (expr_list:REG_EQUAL (const_int 0 [0])
> >           (nil)))
> >
> > ......
> >
> > (insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
> >           (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
> >        (expr_list:REG_DEAD (reg:SI 171)
> >           (nil)))
> > (insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
> >           (reg:SI 163)) 52 {movsi_internal_dsp}
> >        (expr_list:REG_DEAD (reg:SI 163)
> >           (expr_list:REG_DEAD (reg/f:SI 162)
> >               (nil))))
> > (insn 54 53 14 2 (set (reg:SI 171)
> >           (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
> > A32])) ticket151.c:49 52 {movsi_internal_dsp}
> >        (expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
> > [flags 0x2]) [4  S4 A32])
> >           (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
> > [flags 0x2]) [4  S4 A32])
> >               (nil))))
> >
> >
> >      The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
> > in insn 77 with a differerent value.
> >      This may causes reload replacing pseudo 171 with mem/u/c:SI
> > (symbol_ref/u:SI ("*.LC8"), which is wrong.
> >      A proposed patch for this issue, please comment:
> Is it possible it's the conditional just above this one that is incorrect?
> 
>           if (DF_REG_DEF_COUNT (regno) != 1
>                && (! note
>                    || rtx_varies_p (XEXP (note, 0), 0)
>                    || (reg_equiv[regno].replacement
>                        && ! rtx_equal_p (XEXP (note, 0),
> 
> reg_equiv[regno].replacement))))
>              {
>                no_equiv (dest, set, NULL);
>                continue;
>              }
> 
> 
> ISTM the only time a multiply-set register can have a valid REG_EQUIV note is if
> each and every assignment to that pseudo has the same RHS.
> 
> Jeff


Thanks Jeff. Yes, I agree that this is a better place to consider. I am thinking of a better way to fix this.

Vladimir, can you shed light on this and give me some suggestions? Thank you.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-23  2:48   ` Yangfei (Felix)
@ 2014-09-23 10:46     ` Felix Yang
  2014-09-23 10:51       ` Felix Yang
  0 siblings, 1 reply; 27+ messages in thread
From: Felix Yang @ 2014-09-23 10:46 UTC (permalink / raw)
  To: Yangfei (Felix); +Cc: Jeff Law, GCC Patches, vmakarov

Hi,
    Attached please fined the updated patch.

Cheers,
Felix


On Tue, Sep 23, 2014 at 10:48 AM, Yangfei (Felix) <felix.yang@huawei.com> wrote:
>> On 09/22/14 08:40, Felix Yang wrote:
>> > Hi,
>> >
>> >      I find that update_equiv_regs in ira.c sets the wrong EQUIV
>> > reg-note for pseudo with more than one definiton in certain situation.
>> >      Here is a simplified RTL snippet after this function finishs handling:
>> >
>> >   (insn 77 37 78 2 (set (reg:SI 171)
>> >           (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
>> >        (expr_list:REG_EQUAL (const_int 0 [0])
>> >           (nil)))
>> >
>> > ......
>> >
>> > (insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
>> >           (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
>> >        (expr_list:REG_DEAD (reg:SI 171)
>> >           (nil)))
>> > (insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
>> >           (reg:SI 163)) 52 {movsi_internal_dsp}
>> >        (expr_list:REG_DEAD (reg:SI 163)
>> >           (expr_list:REG_DEAD (reg/f:SI 162)
>> >               (nil))))
>> > (insn 54 53 14 2 (set (reg:SI 171)
>> >           (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
>> > A32])) ticket151.c:49 52 {movsi_internal_dsp}
>> >        (expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>> > [flags 0x2]) [4  S4 A32])
>> >           (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>> > [flags 0x2]) [4  S4 A32])
>> >               (nil))))
>> >
>> >
>> >      The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
>> > in insn 77 with a differerent value.
>> >      This may causes reload replacing pseudo 171 with mem/u/c:SI
>> > (symbol_ref/u:SI ("*.LC8"), which is wrong.
>> >      A proposed patch for this issue, please comment:
>> Is it possible it's the conditional just above this one that is incorrect?
>>
>>           if (DF_REG_DEF_COUNT (regno) != 1
>>                && (! note
>>                    || rtx_varies_p (XEXP (note, 0), 0)
>>                    || (reg_equiv[regno].replacement
>>                        && ! rtx_equal_p (XEXP (note, 0),
>>
>> reg_equiv[regno].replacement))))
>>              {
>>                no_equiv (dest, set, NULL);
>>                continue;
>>              }
>>
>>
>> ISTM the only time a multiply-set register can have a valid REG_EQUIV note is if
>> each and every assignment to that pseudo has the same RHS.
>>
>> Jeff
>
>
> Thanks Jeff. Yes, I agree that this is a better place to consider. I am thinking of a better way to fix this.
>
> Vladimir, can you shed light on this and give me some suggestions? Thank you.
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-23 10:46     ` Felix Yang
@ 2014-09-23 10:51       ` Felix Yang
  2014-09-23 17:49         ` Jeff Law
  0 siblings, 1 reply; 27+ messages in thread
From: Felix Yang @ 2014-09-23 10:51 UTC (permalink / raw)
  To: Yangfei (Felix); +Cc: Jeff Law, GCC Patches, vmakarov

[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]

Hi,

    Ignore the previous message.
    Attached please find the updated patch.
    Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog    (revision 215500)
+++ gcc/ChangeLog    (working copy)
@@ -1,3 +1,8 @@
+2014-09-23  Felix Yang  <felix.yang@huawei.com>
+
+    * ira.c (update_equiv_regs): Check all definitions for a multiple-set
+    register to make sure that the RHS have the same value.
+
 2014-09-23  Ilya Enkovich  <ilya.enkovich@intel.com>

     * cfgcleanup.c (try_optimize_cfg): Do not remove label
Index: gcc/ira.c
===================================================================
--- gcc/ira.c    (revision 215500)
+++ gcc/ira.c    (working copy)
@@ -3467,16 +3467,43 @@ update_equiv_regs (void)
       if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
         note = NULL_RTX;

-      if (DF_REG_DEF_COUNT (regno) != 1
-          && (! note
+      if (DF_REG_DEF_COUNT (regno) != 1)
+        {
+          rtx list;
+          bool equal_p = true;
+
+          if (! note
           || rtx_varies_p (XEXP (note, 0), 0)
           || (reg_equiv[regno].replacement
               && ! rtx_equal_p (XEXP (note, 0),
-                    reg_equiv[regno].replacement))))
-        {
-          no_equiv (dest, set, NULL);
-          continue;
+                    reg_equiv[regno].replacement)))
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
+
+          list = reg_equiv[regno].init_insns;
+          for (; list; list = XEXP (list, 1))
+        {
+          rtx note_tmp, insn_tmp;
+          insn_tmp = XEXP (list, 0);
+          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+
+          if (note_tmp == 0
+              || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+            {
+              equal_p = false;
+              break;
+            }
+        }
+
+          if (! equal_p)
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
         }
+
       /* Record this insn as initializing this register.  */
       reg_equiv[regno].init_insns
         = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
Cheers,
Felix


On Tue, Sep 23, 2014 at 6:45 PM, Felix Yang <fei.yang0953@gmail.com> wrote:
> Hi,
>     Attached please fined the updated patch.
>
> Cheers,
> Felix
>
>
> On Tue, Sep 23, 2014 at 10:48 AM, Yangfei (Felix) <felix.yang@huawei.com> wrote:
>>> On 09/22/14 08:40, Felix Yang wrote:
>>> > Hi,
>>> >
>>> >      I find that update_equiv_regs in ira.c sets the wrong EQUIV
>>> > reg-note for pseudo with more than one definiton in certain situation.
>>> >      Here is a simplified RTL snippet after this function finishs handling:
>>> >
>>> >   (insn 77 37 78 2 (set (reg:SI 171)
>>> >           (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
>>> >        (expr_list:REG_EQUAL (const_int 0 [0])
>>> >           (nil)))
>>> >
>>> > ......
>>> >
>>> > (insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
>>> >           (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
>>> >        (expr_list:REG_DEAD (reg:SI 171)
>>> >           (nil)))
>>> > (insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
>>> >           (reg:SI 163)) 52 {movsi_internal_dsp}
>>> >        (expr_list:REG_DEAD (reg:SI 163)
>>> >           (expr_list:REG_DEAD (reg/f:SI 162)
>>> >               (nil))))
>>> > (insn 54 53 14 2 (set (reg:SI 171)
>>> >           (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
>>> > A32])) ticket151.c:49 52 {movsi_internal_dsp}
>>> >        (expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>>> > [flags 0x2]) [4  S4 A32])
>>> >           (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>>> > [flags 0x2]) [4  S4 A32])
>>> >               (nil))))
>>> >
>>> >
>>> >      The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
>>> > in insn 77 with a differerent value.
>>> >      This may causes reload replacing pseudo 171 with mem/u/c:SI
>>> > (symbol_ref/u:SI ("*.LC8"), which is wrong.
>>> >      A proposed patch for this issue, please comment:
>>> Is it possible it's the conditional just above this one that is incorrect?
>>>
>>>           if (DF_REG_DEF_COUNT (regno) != 1
>>>                && (! note
>>>                    || rtx_varies_p (XEXP (note, 0), 0)
>>>                    || (reg_equiv[regno].replacement
>>>                        && ! rtx_equal_p (XEXP (note, 0),
>>>
>>> reg_equiv[regno].replacement))))
>>>              {
>>>                no_equiv (dest, set, NULL);
>>>                continue;
>>>              }
>>>
>>>
>>> ISTM the only time a multiply-set register can have a valid REG_EQUIV note is if
>>> each and every assignment to that pseudo has the same RHS.
>>>
>>> Jeff
>>
>>
>> Thanks Jeff. Yes, I agree that this is a better place to consider. I am thinking of a better way to fix this.
>>
>> Vladimir, can you shed light on this and give me some suggestions? Thank you.
>>

[-- Attachment #2: ira-update-equiv-regs.diff --]
[-- Type: text/plain, Size: 1867 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 215500)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2014-09-23  Felix Yang  <felix.yang@huawei.com>
+
+	* ira.c (update_equiv_regs): Check all definitions for a multiple-set
+	register to make sure that the RHS have the same value.
+
 2014-09-23  Ilya Enkovich  <ilya.enkovich@intel.com>
 
 	* cfgcleanup.c (try_optimize_cfg): Do not remove label
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 215500)
+++ gcc/ira.c	(working copy)
@@ -3467,16 +3467,43 @@ update_equiv_regs (void)
 	  if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 	    note = NULL_RTX;
 
-	  if (DF_REG_DEF_COUNT (regno) != 1
-	      && (! note
+	  if (DF_REG_DEF_COUNT (regno) != 1)
+	    {
+	      rtx list;
+	      bool equal_p = true;
+
+	      if (! note
 		  || rtx_varies_p (XEXP (note, 0), 0)
 		  || (reg_equiv[regno].replacement
 		      && ! rtx_equal_p (XEXP (note, 0),
-					reg_equiv[regno].replacement))))
-	    {
-	      no_equiv (dest, set, NULL);
-	      continue;
+					reg_equiv[regno].replacement)))
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
+
+	      list = reg_equiv[regno].init_insns;
+	      for (; list; list = XEXP (list, 1))
+		{
+		  rtx note_tmp, insn_tmp;
+		  insn_tmp = XEXP (list, 0);
+		  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+
+		  if (note_tmp == 0
+		      || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+		    {
+		      equal_p = false;
+		      break;
+		    }
+		}
+
+	      if (! equal_p)
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
 	    }
+
 	  /* Record this insn as initializing this register.  */
 	  reg_equiv[regno].init_insns
 	    = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-23 10:51       ` Felix Yang
@ 2014-09-23 17:49         ` Jeff Law
  2014-09-24 12:07           ` Felix Yang
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Law @ 2014-09-23 17:49 UTC (permalink / raw)
  To: Felix Yang, Yangfei (Felix); +Cc: GCC Patches, vmakarov

On 09/23/14 04:51, Felix Yang wrote:
> Hi,
>
>      Ignore the previous message.
>      Attached please find the updated patch.
>      Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog    (revision 215500)
> +++ gcc/ChangeLog    (working copy)
> @@ -1,3 +1,8 @@
> +2014-09-23  Felix Yang  <felix.yang@huawei.com>
> +
> +    * ira.c (update_equiv_regs): Check all definitions for a multiple-set
> +    register to make sure that the RHS have the same value.
> +
>   2014-09-23  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>       * cfgcleanup.c (try_optimize_cfg): Do not remove label
> Index: gcc/ira.c
> ===================================================================
> --- gcc/ira.c    (revision 215500)
> +++ gcc/ira.c    (working copy)
> @@ -3467,16 +3467,43 @@ update_equiv_regs (void)
>         if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>           note = NULL_RTX;
>
> -      if (DF_REG_DEF_COUNT (regno) != 1
> -          && (! note
> +      if (DF_REG_DEF_COUNT (regno) != 1)
> +        {
> +          rtx list;
This should probably be "rtx_insn_list list".

> +          list = reg_equiv[regno].init_insns;
> +          for (; list; list = XEXP (list, 1))
Please use the next/insn member functions of the rtx_insn_list rather 
than the old style XEXP accessor macros.


> +        {
> +          rtx note_tmp, insn_tmp;
> +          insn_tmp = XEXP (list, 0);
> +          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
> +
> +          if (note_tmp == 0
> +              || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
Under what conditions did you find an insn on this list that did not 
have a note?  There's a deeper question I'm getting to, but let's start 
here.

Rather than use "== 0", if you have an RTX use "== NULL_RTX".  That 
applies to the note_tmp == 0 test above.


Can you make the edits noted above & answer the question WRT insns on 
the reg_equiv[].init_insns list without notes and repost?

Thanks,
Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-23 17:49         ` Jeff Law
@ 2014-09-24 12:07           ` Felix Yang
  2014-09-24 21:56             ` Felix Yang
  2014-09-25 18:57             ` [PATCH " Jeff Law
  0 siblings, 2 replies; 27+ messages in thread
From: Felix Yang @ 2014-09-24 12:07 UTC (permalink / raw)
  To: Jeff Law; +Cc: Yangfei (Felix), GCC Patches, vmakarov

[-- Attachment #1: Type: text/plain, Size: 7312 bytes --]

Hi Jeff,

    Thanks for the comments. I updated the patch adding some enhancements.
    Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.

    Three points:
    1. For multiple-set register, it is not qualified to have a equiv
note once it is marked by no_equiv. The patch is updated with
       this consideration.
    2. For the rtx_insn_list new interface, I noticed that the old
style XEXP accessor macros is still used in function no_equiv.
       And I choose to the old style macros with this patch and should
come up with another patch to fix this issue, OK?
    3. For the conditions that an insn on the init_insns list which
did not have a note, I reconsider this and find that this can
       never happens. So I replaced the check with a gcc assertion.


Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog    (revision 215550)
+++ gcc/ChangeLog    (working copy)
@@ -1,3 +1,11 @@
+2014-09-24  Felix Yang  <felix.yang@huawei.com>
+
+    * ira.c (struct equivalence): Add no_equiv member.
+    (no_equiv): Set no_equiv of struct equivalence if register is marked
+    as having no known equivalence.
+    (update_equiv_regs): Check all definitions for a multiple-set
+    register to make sure that the RHS have the same value.
+
 2014-09-24  Jakub Jelinek  <jakub@redhat.com>

     PR sanitizer/63316
Index: gcc/ira.c
===================================================================
--- gcc/ira.c    (revision 215550)
+++ gcc/ira.c    (working copy)
@@ -2900,6 +2900,8 @@ struct equivalence
   /* Set when an attempt should be made to replace a register
      with the associated src_p entry.  */
   char replace;
+  /* Set if this register has no known equivalence.  */
+  char no_equiv;
 };

 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list == const0_rtx)
     return;
@@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
     return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = XEXP (list, 1))
     {
       rtx insn = XEXP (list, 0);
       remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
@@ -3373,7 +3376,7 @@ update_equiv_regs (void)

       /* If this insn contains more (or less) than a single SET,
          only mark all destinations as having no known equivalence.  */
-      if (set == 0)
+      if (set == NULL_RTX)
         {
           note_stores (PATTERN (insn), no_equiv, NULL);
           continue;
@@ -3467,16 +3470,48 @@ update_equiv_regs (void)
       if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
         note = NULL_RTX;

-      if (DF_REG_DEF_COUNT (regno) != 1
-          && (! note
+      if (DF_REG_DEF_COUNT (regno) != 1)
+        {
+          rtx list;
+          bool equal_p = true;
+
+              /* Check if it is possible that this multiple-set register has
+         a known equivalence.  */
+          if (reg_equiv[regno].no_equiv)
+        continue;
+
+          if (! note
           || rtx_varies_p (XEXP (note, 0), 0)
           || (reg_equiv[regno].replacement
               && ! rtx_equal_p (XEXP (note, 0),
-                    reg_equiv[regno].replacement))))
-        {
-          no_equiv (dest, set, NULL);
-          continue;
+                    reg_equiv[regno].replacement)))
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
+
+          list = reg_equiv[regno].init_insns;
+          for (; list; list = XEXP (list, 1))
+        {
+          rtx note_tmp, insn_tmp;
+
+          insn_tmp = XEXP (list, 0);
+          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+          gcc_assert (note_tmp);
+          if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+            {
+              equal_p = false;
+              break;
+            }
+        }
+
+          if (! equal_p)
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
         }
+
       /* Record this insn as initializing this register.  */
       reg_equiv[regno].init_insns
         = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3505,10 +3540,9 @@ update_equiv_regs (void)
          a register used only in one basic block from a MEM.  If so, and the
          MEM remains unchanged for the life of the register, add a REG_EQUIV
          note.  */
-
       note = find_reg_note (insn, REG_EQUIV, NULL_RTX);

-      if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+      if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
           && MEM_P (SET_SRC (set))
           && validate_equiv_mem (insn, dest, SET_SRC (set)))
         note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
Cheers,
Felix


On Wed, Sep 24, 2014 at 1:49 AM, Jeff Law <law@redhat.com> wrote:
> On 09/23/14 04:51, Felix Yang wrote:
>>
>> Hi,
>>
>>      Ignore the previous message.
>>      Attached please find the updated patch.
>>      Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for
>> trunk.
>>
>> Index: gcc/ChangeLog
>> ===================================================================
>> --- gcc/ChangeLog    (revision 215500)
>> +++ gcc/ChangeLog    (working copy)
>> @@ -1,3 +1,8 @@
>> +2014-09-23  Felix Yang  <felix.yang@huawei.com>
>> +
>> +    * ira.c (update_equiv_regs): Check all definitions for a multiple-set
>> +    register to make sure that the RHS have the same value.
>> +
>>   2014-09-23  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>       * cfgcleanup.c (try_optimize_cfg): Do not remove label
>> Index: gcc/ira.c
>> ===================================================================
>> --- gcc/ira.c    (revision 215500)
>> +++ gcc/ira.c    (working copy)
>> @@ -3467,16 +3467,43 @@ update_equiv_regs (void)
>>         if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>>           note = NULL_RTX;
>>
>> -      if (DF_REG_DEF_COUNT (regno) != 1
>> -          && (! note
>> +      if (DF_REG_DEF_COUNT (regno) != 1)
>> +        {
>> +          rtx list;
>
> This should probably be "rtx_insn_list list".
>
>> +          list = reg_equiv[regno].init_insns;
>> +          for (; list; list = XEXP (list, 1))
>
> Please use the next/insn member functions of the rtx_insn_list rather than
> the old style XEXP accessor macros.
>
>
>> +        {
>> +          rtx note_tmp, insn_tmp;
>> +          insn_tmp = XEXP (list, 0);
>> +          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
>> +
>> +          if (note_tmp == 0
>> +              || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
>
> Under what conditions did you find an insn on this list that did not have a
> note?  There's a deeper question I'm getting to, but let's start here.
>
> Rather than use "== 0", if you have an RTX use "== NULL_RTX".  That applies
> to the note_tmp == 0 test above.
>
>
> Can you make the edits noted above & answer the question WRT insns on the
> reg_equiv[].init_insns list without notes and repost?
>
> Thanks,
> Jeff

[-- Attachment #2: ira-update-equiv-regs-new.diff --]
[-- Type: text/plain, Size: 3943 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 215550)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2014-09-24  Felix Yang  <felix.yang@huawei.com>
+
+	* ira.c (struct equivalence): Add no_equiv member.
+	(no_equiv): Set no_equiv of struct equivalence if register is marked
+	as having no known equivalence.
+	(update_equiv_regs): Check all definitions for a multiple-set
+	register to make sure that the RHS have the same value.
+
 2014-09-24  Jakub Jelinek  <jakub@redhat.com>
 
 	PR sanitizer/63316
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 215550)
+++ gcc/ira.c	(working copy)
@@ -2900,6 +2900,8 @@ struct equivalence
   /* Set when an attempt should be made to replace a register
      with the associated src_p entry.  */
   char replace;
+  /* Set if this register has no known equivalence.  */
+  char no_equiv;
 };
 
 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list == const0_rtx)
     return;
@@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
     return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = XEXP (list, 1))
     {
       rtx insn = XEXP (list, 0);
       remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
@@ -3373,7 +3376,7 @@ update_equiv_regs (void)
 
 	  /* If this insn contains more (or less) than a single SET,
 	     only mark all destinations as having no known equivalence.  */
-	  if (set == 0)
+	  if (set == NULL_RTX)
 	    {
 	      note_stores (PATTERN (insn), no_equiv, NULL);
 	      continue;
@@ -3467,16 +3470,48 @@ update_equiv_regs (void)
 	  if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 	    note = NULL_RTX;
 
-	  if (DF_REG_DEF_COUNT (regno) != 1
-	      && (! note
+	  if (DF_REG_DEF_COUNT (regno) != 1)
+	    {
+	      rtx list;
+	      bool equal_p = true;
+
+              /* Check if it is possible that this multiple-set register has
+		 a known equivalence.  */
+	      if (reg_equiv[regno].no_equiv)
+		continue;
+
+	      if (! note
 		  || rtx_varies_p (XEXP (note, 0), 0)
 		  || (reg_equiv[regno].replacement
 		      && ! rtx_equal_p (XEXP (note, 0),
-					reg_equiv[regno].replacement))))
-	    {
-	      no_equiv (dest, set, NULL);
-	      continue;
+					reg_equiv[regno].replacement)))
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
+
+	      list = reg_equiv[regno].init_insns;
+	      for (; list; list = XEXP (list, 1))
+		{
+		  rtx note_tmp, insn_tmp;
+
+		  insn_tmp = XEXP (list, 0);
+		  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+		  gcc_assert (note_tmp);
+		  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+		    {
+		      equal_p = false;
+		      break;
+		    }
+		}
+
+	      if (! equal_p)
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
 	    }
+
 	  /* Record this insn as initializing this register.  */
 	  reg_equiv[regno].init_insns
 	    = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3505,10 +3540,9 @@ update_equiv_regs (void)
 	     a register used only in one basic block from a MEM.  If so, and the
 	     MEM remains unchanged for the life of the register, add a REG_EQUIV
 	     note.  */
-
 	  note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
 
-	  if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+	  if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
 	      && MEM_P (SET_SRC (set))
 	      && validate_equiv_mem (insn, dest, SET_SRC (set)))
 	    note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-24 12:07           ` Felix Yang
@ 2014-09-24 21:56             ` Felix Yang
  2014-09-25  4:07               ` [PING PATCH " Yangfei (Felix)
  2014-09-25 18:57             ` [PATCH " Jeff Law
  1 sibling, 1 reply; 27+ messages in thread
From: Felix Yang @ 2014-09-24 21:56 UTC (permalink / raw)
  To: Jeff Law; +Cc: Yangfei (Felix), GCC Patches, vmakarov

PING ?
Cheers,
Felix


On Wed, Sep 24, 2014 at 8:07 PM, Felix Yang <fei.yang0953@gmail.com> wrote:
> Hi Jeff,
>
>     Thanks for the comments. I updated the patch adding some enhancements.
>     Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.
>
>     Three points:
>     1. For multiple-set register, it is not qualified to have a equiv
> note once it is marked by no_equiv. The patch is updated with
>        this consideration.
>     2. For the rtx_insn_list new interface, I noticed that the old
> style XEXP accessor macros is still used in function no_equiv.
>        And I choose to the old style macros with this patch and should
> come up with another patch to fix this issue, OK?
>     3. For the conditions that an insn on the init_insns list which
> did not have a note, I reconsider this and find that this can
>        never happens. So I replaced the check with a gcc assertion.
>
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog    (revision 215550)
> +++ gcc/ChangeLog    (working copy)
> @@ -1,3 +1,11 @@
> +2014-09-24  Felix Yang  <felix.yang@huawei.com>
> +
> +    * ira.c (struct equivalence): Add no_equiv member.
> +    (no_equiv): Set no_equiv of struct equivalence if register is marked
> +    as having no known equivalence.
> +    (update_equiv_regs): Check all definitions for a multiple-set
> +    register to make sure that the RHS have the same value.
> +
>  2014-09-24  Jakub Jelinek  <jakub@redhat.com>
>
>      PR sanitizer/63316
> Index: gcc/ira.c
> ===================================================================
> --- gcc/ira.c    (revision 215550)
> +++ gcc/ira.c    (working copy)
> @@ -2900,6 +2900,8 @@ struct equivalence
>    /* Set when an attempt should be made to replace a register
>       with the associated src_p entry.  */
>    char replace;
> +  /* Set if this register has no known equivalence.  */
> +  char no_equiv;
>  };
>
>  /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
> @@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>    if (!REG_P (reg))
>      return;
>    regno = REGNO (reg);
> +  reg_equiv[regno].no_equiv = 1;
>    list = reg_equiv[regno].init_insns;
>    if (list == const0_rtx)
>      return;
> @@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>      return;
>    ira_reg_equiv[regno].defined_p = false;
>    ira_reg_equiv[regno].init_insns = NULL;
> -  for (; list; list =  XEXP (list, 1))
> +  for (; list; list = XEXP (list, 1))
>      {
>        rtx insn = XEXP (list, 0);
>        remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
> @@ -3373,7 +3376,7 @@ update_equiv_regs (void)
>
>        /* If this insn contains more (or less) than a single SET,
>           only mark all destinations as having no known equivalence.  */
> -      if (set == 0)
> +      if (set == NULL_RTX)
>          {
>            note_stores (PATTERN (insn), no_equiv, NULL);
>            continue;
> @@ -3467,16 +3470,48 @@ update_equiv_regs (void)
>        if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>          note = NULL_RTX;
>
> -      if (DF_REG_DEF_COUNT (regno) != 1
> -          && (! note
> +      if (DF_REG_DEF_COUNT (regno) != 1)
> +        {
> +          rtx list;
> +          bool equal_p = true;
> +
> +              /* Check if it is possible that this multiple-set register has
> +         a known equivalence.  */
> +          if (reg_equiv[regno].no_equiv)
> +        continue;
> +
> +          if (! note
>            || rtx_varies_p (XEXP (note, 0), 0)
>            || (reg_equiv[regno].replacement
>                && ! rtx_equal_p (XEXP (note, 0),
> -                    reg_equiv[regno].replacement))))
> -        {
> -          no_equiv (dest, set, NULL);
> -          continue;
> +                    reg_equiv[regno].replacement)))
> +        {
> +          no_equiv (dest, set, NULL);
> +          continue;
> +        }
> +
> +          list = reg_equiv[regno].init_insns;
> +          for (; list; list = XEXP (list, 1))
> +        {
> +          rtx note_tmp, insn_tmp;
> +
> +          insn_tmp = XEXP (list, 0);
> +          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
> +          gcc_assert (note_tmp);
> +          if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
> +            {
> +              equal_p = false;
> +              break;
> +            }
> +        }
> +
> +          if (! equal_p)
> +        {
> +          no_equiv (dest, set, NULL);
> +          continue;
> +        }
>          }
> +
>        /* Record this insn as initializing this register.  */
>        reg_equiv[regno].init_insns
>          = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
> @@ -3505,10 +3540,9 @@ update_equiv_regs (void)
>           a register used only in one basic block from a MEM.  If so, and the
>           MEM remains unchanged for the life of the register, add a REG_EQUIV
>           note.  */
> -
>        note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
>
> -      if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
> +      if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
>            && MEM_P (SET_SRC (set))
>            && validate_equiv_mem (insn, dest, SET_SRC (set)))
>          note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
> Cheers,
> Felix
>
>
> On Wed, Sep 24, 2014 at 1:49 AM, Jeff Law <law@redhat.com> wrote:
>> On 09/23/14 04:51, Felix Yang wrote:
>>>
>>> Hi,
>>>
>>>      Ignore the previous message.
>>>      Attached please find the updated patch.
>>>      Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for
>>> trunk.
>>>
>>> Index: gcc/ChangeLog
>>> ===================================================================
>>> --- gcc/ChangeLog    (revision 215500)
>>> +++ gcc/ChangeLog    (working copy)
>>> @@ -1,3 +1,8 @@
>>> +2014-09-23  Felix Yang  <felix.yang@huawei.com>
>>> +
>>> +    * ira.c (update_equiv_regs): Check all definitions for a multiple-set
>>> +    register to make sure that the RHS have the same value.
>>> +
>>>   2014-09-23  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>
>>>       * cfgcleanup.c (try_optimize_cfg): Do not remove label
>>> Index: gcc/ira.c
>>> ===================================================================
>>> --- gcc/ira.c    (revision 215500)
>>> +++ gcc/ira.c    (working copy)
>>> @@ -3467,16 +3467,43 @@ update_equiv_regs (void)
>>>         if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>>>           note = NULL_RTX;
>>>
>>> -      if (DF_REG_DEF_COUNT (regno) != 1
>>> -          && (! note
>>> +      if (DF_REG_DEF_COUNT (regno) != 1)
>>> +        {
>>> +          rtx list;
>>
>> This should probably be "rtx_insn_list list".
>>
>>> +          list = reg_equiv[regno].init_insns;
>>> +          for (; list; list = XEXP (list, 1))
>>
>> Please use the next/insn member functions of the rtx_insn_list rather than
>> the old style XEXP accessor macros.
>>
>>
>>> +        {
>>> +          rtx note_tmp, insn_tmp;
>>> +          insn_tmp = XEXP (list, 0);
>>> +          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
>>> +
>>> +          if (note_tmp == 0
>>> +              || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
>>
>> Under what conditions did you find an insn on this list that did not have a
>> note?  There's a deeper question I'm getting to, but let's start here.
>>
>> Rather than use "== 0", if you have an RTX use "== NULL_RTX".  That applies
>> to the note_tmp == 0 test above.
>>
>>
>> Can you make the edits noted above & answer the question WRT insns on the
>> reg_equiv[].init_insns list without notes and repost?
>>
>> Thanks,
>> Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PING PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-24 21:56             ` Felix Yang
@ 2014-09-25  4:07               ` Yangfei (Felix)
  0 siblings, 0 replies; 27+ messages in thread
From: Yangfei (Felix) @ 2014-09-25  4:07 UTC (permalink / raw)
  To: Felix Yang, Jeff Law, GCC Patches, vmakarov


> 
> PING ?
> Cheers,
> Felix
> 
> 
> On Wed, Sep 24, 2014 at 8:07 PM, Felix Yang <fei.yang0953@gmail.com>
> wrote:
> > Hi Jeff,
> >
> >     Thanks for the comments. I updated the patch adding some
> enhancements.
> >     Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for
> trunk.
> >
> >     Three points:
> >     1. For multiple-set register, it is not qualified to have a equiv
> > note once it is marked by no_equiv. The patch is updated with
> >        this consideration.
> >     2. For the rtx_insn_list new interface, I noticed that the old
> > style XEXP accessor macros is still used in function no_equiv.
> >        And I choose to the old style macros with this patch and should
> > come up with another patch to fix this issue, OK?
> >     3. For the conditions that an insn on the init_insns list which
> > did not have a note, I reconsider this and find that this can
> >        never happens. So I replaced the check with a gcc assertion.
> >
> >
> > Index: gcc/ChangeLog
> >
> ================================================================
> ===
> > --- gcc/ChangeLog    (revision 215550)
> > +++ gcc/ChangeLog    (working copy)
> > @@ -1,3 +1,11 @@
> > +2014-09-24  Felix Yang  <felix.yang@huawei.com>
> > +
> > +    * ira.c (struct equivalence): Add no_equiv member.
> > +    (no_equiv): Set no_equiv of struct equivalence if register is marked
> > +    as having no known equivalence.
> > +    (update_equiv_regs): Check all definitions for a multiple-set
> > +    register to make sure that the RHS have the same value.
> > +
> >  2014-09-24  Jakub Jelinek  <jakub@redhat.com>
> >
> >      PR sanitizer/63316
> > Index: gcc/ira.c
> >
> ================================================================
> ===
> > --- gcc/ira.c    (revision 215550)
> > +++ gcc/ira.c    (working copy)
> > @@ -2900,6 +2900,8 @@ struct equivalence
> >    /* Set when an attempt should be made to replace a register
> >       with the associated src_p entry.  */
> >    char replace;
> > +  /* Set if this register has no known equivalence.  */  char
> > + no_equiv;
> >  };
> >
> >  /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
> > @@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store
> ATTRIBUTE_UNUSE
> >    if (!REG_P (reg))
> >      return;
> >    regno = REGNO (reg);
> > +  reg_equiv[regno].no_equiv = 1;
> >    list = reg_equiv[regno].init_insns;
> >    if (list == const0_rtx)
> >      return;
> > @@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store
> ATTRIBUTE_UNUSE
> >      return;
> >    ira_reg_equiv[regno].defined_p = false;
> >    ira_reg_equiv[regno].init_insns = NULL;
> > -  for (; list; list =  XEXP (list, 1))
> > +  for (; list; list = XEXP (list, 1))
> >      {
> >        rtx insn = XEXP (list, 0);
> >        remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
> > @@ -3373,7 +3376,7 @@ update_equiv_regs (void)
> >
> >        /* If this insn contains more (or less) than a single SET,
> >           only mark all destinations as having no known equivalence.  */
> > -      if (set == 0)
> > +      if (set == NULL_RTX)
> >          {
> >            note_stores (PATTERN (insn), no_equiv, NULL);
> >            continue;
> > @@ -3467,16 +3470,48 @@ update_equiv_regs (void)
> >        if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
> >          note = NULL_RTX;
> >
> > -      if (DF_REG_DEF_COUNT (regno) != 1
> > -          && (! note
> > +      if (DF_REG_DEF_COUNT (regno) != 1)
> > +        {
> > +          rtx list;
> > +          bool equal_p = true;
> > +
> > +              /* Check if it is possible that this multiple-set register has
> > +         a known equivalence.  */
> > +          if (reg_equiv[regno].no_equiv)
> > +        continue;
> > +
> > +          if (! note
> >            || rtx_varies_p (XEXP (note, 0), 0)
> >            || (reg_equiv[regno].replacement
> >                && ! rtx_equal_p (XEXP (note, 0),
> > -                    reg_equiv[regno].replacement))))
> > -        {
> > -          no_equiv (dest, set, NULL);
> > -          continue;
> > +                    reg_equiv[regno].replacement)))
> > +        {
> > +          no_equiv (dest, set, NULL);
> > +          continue;
> > +        }
> > +
> > +          list = reg_equiv[regno].init_insns;
> > +          for (; list; list = XEXP (list, 1))
> > +        {
> > +          rtx note_tmp, insn_tmp;
> > +
> > +          insn_tmp = XEXP (list, 0);
> > +          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
> > +          gcc_assert (note_tmp);
> > +          if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
> > +            {
> > +              equal_p = false;
> > +              break;
> > +            }
> > +        }
> > +
> > +          if (! equal_p)
> > +        {
> > +          no_equiv (dest, set, NULL);
> > +          continue;
> > +        }
> >          }
> > +
> >        /* Record this insn as initializing this register.  */
> >        reg_equiv[regno].init_insns
> >          = gen_rtx_INSN_LIST (VOIDmode, insn,
> > reg_equiv[regno].init_insns); @@ -3505,10 +3540,9 @@ update_equiv_regs
> (void)
> >           a register used only in one basic block from a MEM.  If so, and
> the
> >           MEM remains unchanged for the life of the register, add a
> REG_EQUIV
> >           note.  */
> > -
> >        note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
> >
> > -      if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
> > +      if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >=
> > + NUM_FIXED_BLOCKS
> >            && MEM_P (SET_SRC (set))
> >            && validate_equiv_mem (insn, dest, SET_SRC (set)))
> >          note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx
> > (SET_SRC (set))); Cheers, Felix
> >
> >
> > On Wed, Sep 24, 2014 at 1:49 AM, Jeff Law <law@redhat.com> wrote:
> >> On 09/23/14 04:51, Felix Yang wrote:
> >>>
> >>> Hi,
> >>>
> >>>      Ignore the previous message.
> >>>      Attached please find the updated patch.
> >>>      Bootstrapped on x86_64-suse-linux. Please apply this patch if
> >>> OK for trunk.
> >>>
> >>> Index: gcc/ChangeLog
> >>>
> ================================================================
> ===
> >>> --- gcc/ChangeLog    (revision 215500)
> >>> +++ gcc/ChangeLog    (working copy)
> >>> @@ -1,3 +1,8 @@
> >>> +2014-09-23  Felix Yang  <felix.yang@huawei.com>
> >>> +
> >>> +    * ira.c (update_equiv_regs): Check all definitions for a multiple-set
> >>> +    register to make sure that the RHS have the same value.
> >>> +
> >>>   2014-09-23  Ilya Enkovich  <ilya.enkovich@intel.com>
> >>>
> >>>       * cfgcleanup.c (try_optimize_cfg): Do not remove label
> >>> Index: gcc/ira.c
> >>>
> ================================================================
> ===
> >>> --- gcc/ira.c    (revision 215500)
> >>> +++ gcc/ira.c    (working copy)
> >>> @@ -3467,16 +3467,43 @@ update_equiv_regs (void)
> >>>         if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
> >>>           note = NULL_RTX;
> >>>
> >>> -      if (DF_REG_DEF_COUNT (regno) != 1
> >>> -          && (! note
> >>> +      if (DF_REG_DEF_COUNT (regno) != 1)
> >>> +        {
> >>> +          rtx list;
> >>
> >> This should probably be "rtx_insn_list list".
> >>
> >>> +          list = reg_equiv[regno].init_insns;
> >>> +          for (; list; list = XEXP (list, 1))
> >>
> >> Please use the next/insn member functions of the rtx_insn_list rather
> >> than the old style XEXP accessor macros.
> >>
> >>
> >>> +        {
> >>> +          rtx note_tmp, insn_tmp;
> >>> +          insn_tmp = XEXP (list, 0);
> >>> +          note_tmp = find_reg_note (insn_tmp, REG_EQUAL,
> NULL_RTX);
> >>> +
> >>> +          if (note_tmp == 0
> >>> +              || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp,
> >>> + 0)))
> >>
> >> Under what conditions did you find an insn on this list that did not
> >> have a note?  There's a deeper question I'm getting to, but let's start here.
> >>
> >> Rather than use "== 0", if you have an RTX use "== NULL_RTX".  That
> >> applies to the note_tmp == 0 test above.
> >>
> >>
> >> Can you make the edits noted above & answer the question WRT insns on
> >> the reg_equiv[].init_insns list without notes and repost?
> >>
> >> Thanks,
> >> Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-24 12:07           ` Felix Yang
  2014-09-24 21:56             ` Felix Yang
@ 2014-09-25 18:57             ` Jeff Law
  2014-09-26 13:57               ` Felix Yang
  1 sibling, 1 reply; 27+ messages in thread
From: Jeff Law @ 2014-09-25 18:57 UTC (permalink / raw)
  To: Felix Yang; +Cc: Yangfei (Felix), GCC Patches, vmakarov

On 09/24/14 06:07, Felix Yang wrote:
> Hi Jeff,
>
>      Thanks for the comments. I updated the patch adding some enhancements.
>      Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.
>
>      Three points:
>      1. For multiple-set register, it is not qualified to have a equiv
> note once it is marked by no_equiv. The patch is updated with
>         this consideration.
Correct.

>      2. For the rtx_insn_list new interface, I noticed that the old
> style XEXP accessor macros is still used in function no_equiv.
>         And I choose to the old style macros with this patch and should
> come up with another patch to fix this issue, OK?
I'd rather any new code going in use the updated interfaces.  It's 
certainly OK to have af followup patch which converts more pre-existing 
code to the new interfaces.

>      3. For the conditions that an insn on the init_insns list which
> did not have a note, I reconsider this and find that this can
>         never happens. So I replaced the check with a gcc assertion.
OK.

Also, I should have asked this earlier, do you have an assignment on 
file with the FSF, or does your employer have any kind of blanket 
assignment on file with the FSF?  These changes are large enough to 
require an assignment.


> Index: gcc/ira.c
> ===================================================================
> --- gcc/ira.c    (revision 215550)
> +++ gcc/ira.c    (working copy)
> @@ -2900,6 +2900,8 @@ struct equivalence
>     /* Set when an attempt should be made to replace a register
>        with the associated src_p entry.  */
>     char replace;
> +  /* Set if this register has no known equivalence.  */
> +  char no_equiv;
>   };
As a follow-up, can you turn is_arg_equivalence, replace and no_equiv 
into boolean bitfields and turn loop_depth into a short (to match 
assumptions elsewhere in GCC).


The point is to get better packing of these objects and ultimately use 
less memory.

> +
> +              /* Check if it is possible that this multiple-set register has
> +         a known equivalence.  */
> +          if (reg_equiv[regno].no_equiv)
> +        continue;
This comment is a bit confusing.  Please consider something like

/* If we have already processed this pseudo and determined it
    can not have an equivalence, then honor that decision.  */


Do you have a testcase we can add to the regression suite?  If at all 
possible please include one.    An execution test would be best, but you 
could also scan the RTL for bogus REG_EQUIV notes.

Please update to use the new type and interfaces for list walking the 
init_insns list.

Finally, you need to verify your patch will bootstrap and not cause any 
regressions in the testsuite.  If you're unsure how to do that, let me know.

I think we'll be ready to go once those tasks are complete.


jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-25 18:57             ` [PATCH " Jeff Law
@ 2014-09-26 13:57               ` Felix Yang
  2014-09-26 21:03                 ` Jeff Law
  0 siblings, 1 reply; 27+ messages in thread
From: Felix Yang @ 2014-09-26 13:57 UTC (permalink / raw)
  To: Jeff Law; +Cc: Yangfei (Felix), GCC Patches, vmakarov

[-- Attachment #1: Type: text/plain, Size: 9837 bytes --]

Hi Jeff,

    Thanks for the suggestions. I updated the patch accordingly.

    1. Both my employer(Huawei) and I have signed the copyright
assignments with FSF.
        These assignments are already sent via post two days ago and
hopefully should reach FSF in one week.
        Maybe it's OK to commit this patch now?

     2. I am not turning member loop_depth of struct equivalence into
short integer as GCC API such as bb_loop_depth
         returns a loop's depth as a 32-bit interger.

     3. I find it's kind of difficult to use the new type and
interfaces for list walking the init_insns list for this patch.
        The type of init_insns list is rtx, not rtl_insn_list *. Seems
we need to change a lot in order to use the new interface.
        Not clear about the reason why it is not adjusted when we are
transferring to the new interface.
        Anyway, I think it's better to have another patch fix that issue. OK?

     4. This bug is only reproduceable with my local customized GCC
version. So I don't have a testcase then.

     5. This patch bootstrapped on x86_64-suse-linux and reg-tested,
There are no regressions with this patch.
         Regression test summary with or without the patch:

        === gcc Summary ===

# of expected passes        107986
# of unexpected failures    348
# of unexpected successes    33
# of expected failures        262
# of unsupported tests        2089
/home/yangfei/gcc-devel/gcc-build/gcc/xgcc  version 5.0.0 20140924
(experimental) (GCC)
--
        === g++ Summary ===

# of expected passes        87415
# of unexpected failures    276
# of expected failures        266
# of unsupported tests        3203
/home/yangfei/gcc-devel/gcc-build/gcc/testsuite/g++/../../xg++
version 5.0.0 20140924 (experimental) (GCC)

--
        === libatomic Summary ===

# of expected passes        54
        === libgomp tests ===


Running target unix

        === libgomp Summary ===

# of expected passes        693
        === libitm tests ===


Running target unix

        === libitm Summary ===

# of expected passes        26
# of expected failures        3
# of unsupported tests        1
        === libstdc++ tests ===


+++ gcc/ChangeLog    (working copy)
@@ -1,3 +1,13 @@
+2014-09-26  Felix Yang  <felix.yang@huawei.com>
+        Jeff Law  <law@redhat.com>
+
+    * ira.c (struct equivalence): Change member "is_arg_equivalence"
and "replace"
+    into boolean bitfields; add new member "no_equiv" and "reserved".
+    (no_equiv): Set no_equiv of struct equivalence if register is marked
+    as having no known equivalence.
+    (update_equiv_regs): Check all definitions for a multiple-set
+    register to make sure that the RHS have the same value.
+
 2014-09-26  Martin Liska  <mliska@suse.cz>

     * cgraph.c (cgraph_node::release_body): New argument keep_arguments
Index: gcc/ira.c
===================================================================
--- gcc/ira.c    (revision 215640)
+++ gcc/ira.c    (working copy)
@@ -2896,10 +2896,13 @@ struct equivalence
      to be present within the same loop (or in an inner loop).  */
   int loop_depth;
   /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
   /* Set when an attempt should be made to replace a register
      with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
+  unsigned char reserved : 5;
 };

 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3247,6 +3250,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list == const0_rtx)
     return;
@@ -3258,7 +3262,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
     return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = XEXP (list, 1))
     {
       rtx insn = XEXP (list, 0);
       remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
@@ -3373,7 +3377,7 @@ update_equiv_regs (void)

       /* If this insn contains more (or less) than a single SET,
          only mark all destinations as having no known equivalence.  */
-      if (set == 0)
+      if (set == NULL_RTX)
         {
           note_stores (PATTERN (insn), no_equiv, NULL);
           continue;
@@ -3467,16 +3471,48 @@ update_equiv_regs (void)
       if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
         note = NULL_RTX;

-      if (DF_REG_DEF_COUNT (regno) != 1
-          && (! note
+      if (DF_REG_DEF_COUNT (regno) != 1)
+        {
+          rtx list;
+          bool equal_p = true;
+
+          /* If we have already processed this pseudo and determined it
+         can not have an equivalence, then honor that decision.  */
+          if (reg_equiv[regno].no_equiv)
+        continue;
+
+          if (! note
           || rtx_varies_p (XEXP (note, 0), 0)
           || (reg_equiv[regno].replacement
               && ! rtx_equal_p (XEXP (note, 0),
-                    reg_equiv[regno].replacement))))
-        {
-          no_equiv (dest, set, NULL);
-          continue;
+                    reg_equiv[regno].replacement)))
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
+
+          list = reg_equiv[regno].init_insns;
+          for (; list; list = XEXP (list, 1))
+        {
+          rtx note_tmp, insn_tmp;
+
+          insn_tmp = XEXP (list, 0);
+          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+          gcc_assert (note_tmp);
+          if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+            {
+              equal_p = false;
+              break;
+            }
+        }
+
+          if (! equal_p)
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
         }
+
       /* Record this insn as initializing this register.  */
       reg_equiv[regno].init_insns
         = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3505,10 +3541,9 @@ update_equiv_regs (void)
          a register used only in one basic block from a MEM.  If so, and the
          MEM remains unchanged for the life of the register, add a REG_EQUIV
          note.  */
-
       note = find_reg_note (insn, REG_EQUIV, NULL_RTX);

-      if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+      if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
           && MEM_P (SET_SRC (set))
           && validate_equiv_mem (insn, dest, SET_SRC (set)))
         note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));

Cheers,
Felix


On Fri, Sep 26, 2014 at 2:57 AM, Jeff Law <law@redhat.com> wrote:
> On 09/24/14 06:07, Felix Yang wrote:
>>
>> Hi Jeff,
>>
>>      Thanks for the comments. I updated the patch adding some
>> enhancements.
>>      Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for
>> trunk.
>>
>>      Three points:
>>      1. For multiple-set register, it is not qualified to have a equiv
>> note once it is marked by no_equiv. The patch is updated with
>>         this consideration.
>
> Correct.
>
>>      2. For the rtx_insn_list new interface, I noticed that the old
>> style XEXP accessor macros is still used in function no_equiv.
>>         And I choose to the old style macros with this patch and should
>> come up with another patch to fix this issue, OK?
>
> I'd rather any new code going in use the updated interfaces.  It's certainly
> OK to have af followup patch which converts more pre-existing code to the
> new interfaces.
>
>>      3. For the conditions that an insn on the init_insns list which
>> did not have a note, I reconsider this and find that this can
>>         never happens. So I replaced the check with a gcc assertion.
>
> OK.
>
> Also, I should have asked this earlier, do you have an assignment on file
> with the FSF, or does your employer have any kind of blanket assignment on
> file with the FSF?  These changes are large enough to require an assignment.
>
>
>> Index: gcc/ira.c
>> ===================================================================
>> --- gcc/ira.c    (revision 215550)
>> +++ gcc/ira.c    (working copy)
>> @@ -2900,6 +2900,8 @@ struct equivalence
>>     /* Set when an attempt should be made to replace a register
>>        with the associated src_p entry.  */
>>     char replace;
>> +  /* Set if this register has no known equivalence.  */
>> +  char no_equiv;
>>   };
>
> As a follow-up, can you turn is_arg_equivalence, replace and no_equiv into
> boolean bitfields and turn loop_depth into a short (to match assumptions
> elsewhere in GCC).
>
>
> The point is to get better packing of these objects and ultimately use less
> memory.
>
>> +
>> +              /* Check if it is possible that this multiple-set register
>> has
>> +         a known equivalence.  */
>> +          if (reg_equiv[regno].no_equiv)
>> +        continue;
>
> This comment is a bit confusing.  Please consider something like
>
> /* If we have already processed this pseudo and determined it
>    can not have an equivalence, then honor that decision.  */
>
>
> Do you have a testcase we can add to the regression suite?  If at all
> possible please include one.    An execution test would be best, but you
> could also scan the RTL for bogus REG_EQUIV notes.
>
> Please update to use the new type and interfaces for list walking the
> init_insns list.
>
> Finally, you need to verify your patch will bootstrap and not cause any
> regressions in the testsuite.  If you're unsure how to do that, let me know.
>
> I think we'll be ready to go once those tasks are complete.
>
>
> jeff
>

[-- Attachment #2: ira-update-equiv-regs-review.diff --]
[-- Type: text/plain, Size: 4435 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 215640)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,13 @@
+2014-09-26  Felix Yang  <felix.yang@huawei.com>
+	    Jeff Law  <law@redhat.com>
+
+	* ira.c (struct equivalence): Change member "is_arg_equivalence" and "replace"
+	into boolean bitfields; add new member "no_equiv" and "reserved".
+	(no_equiv): Set no_equiv of struct equivalence if register is marked
+	as having no known equivalence.
+	(update_equiv_regs): Check all definitions for a multiple-set
+	register to make sure that the RHS have the same value.
+
 2014-09-26  Martin Liska  <mliska@suse.cz>
 
 	* cgraph.c (cgraph_node::release_body): New argument keep_arguments
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 215640)
+++ gcc/ira.c	(working copy)
@@ -2896,10 +2896,13 @@ struct equivalence
      to be present within the same loop (or in an inner loop).  */
   int loop_depth;
   /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
   /* Set when an attempt should be made to replace a register
      with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
+  unsigned char reserved : 5;
 };
 
 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3247,6 +3250,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list == const0_rtx)
     return;
@@ -3258,7 +3262,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
     return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = XEXP (list, 1))
     {
       rtx insn = XEXP (list, 0);
       remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
@@ -3373,7 +3377,7 @@ update_equiv_regs (void)
 
 	  /* If this insn contains more (or less) than a single SET,
 	     only mark all destinations as having no known equivalence.  */
-	  if (set == 0)
+	  if (set == NULL_RTX)
 	    {
 	      note_stores (PATTERN (insn), no_equiv, NULL);
 	      continue;
@@ -3467,16 +3471,48 @@ update_equiv_regs (void)
 	  if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 	    note = NULL_RTX;
 
-	  if (DF_REG_DEF_COUNT (regno) != 1
-	      && (! note
+	  if (DF_REG_DEF_COUNT (regno) != 1)
+	    {
+	      rtx list;
+	      bool equal_p = true;
+
+	      /* If we have already processed this pseudo and determined it
+		 can not have an equivalence, then honor that decision.  */
+	      if (reg_equiv[regno].no_equiv)
+		continue;
+
+	      if (! note
 		  || rtx_varies_p (XEXP (note, 0), 0)
 		  || (reg_equiv[regno].replacement
 		      && ! rtx_equal_p (XEXP (note, 0),
-					reg_equiv[regno].replacement))))
-	    {
-	      no_equiv (dest, set, NULL);
-	      continue;
+					reg_equiv[regno].replacement)))
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
+
+	      list = reg_equiv[regno].init_insns;
+	      for (; list; list = XEXP (list, 1))
+		{
+		  rtx note_tmp, insn_tmp;
+
+		  insn_tmp = XEXP (list, 0);
+		  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+		  gcc_assert (note_tmp);
+		  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+		    {
+		      equal_p = false;
+		      break;
+		    }
+		}
+
+	      if (! equal_p)
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
 	    }
+
 	  /* Record this insn as initializing this register.  */
 	  reg_equiv[regno].init_insns
 	    = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3505,10 +3541,9 @@ update_equiv_regs (void)
 	     a register used only in one basic block from a MEM.  If so, and the
 	     MEM remains unchanged for the life of the register, add a REG_EQUIV
 	     note.  */
-
 	  note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
 
-	  if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+	  if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
 	      && MEM_P (SET_SRC (set))
 	      && validate_equiv_mem (insn, dest, SET_SRC (set)))
 	    note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-26 13:57               ` Felix Yang
@ 2014-09-26 21:03                 ` Jeff Law
  2014-09-27 14:48                   ` Felix Yang
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Law @ 2014-09-26 21:03 UTC (permalink / raw)
  To: Felix Yang; +Cc: Yangfei (Felix), GCC Patches, vmakarov

On 09/26/14 07:57, Felix Yang wrote:
> Hi Jeff,
>
>      Thanks for the suggestions. I updated the patch accordingly.
>
>      1. Both my employer(Huawei) and I have signed the copyright
> assignments with FSF.
>          These assignments are already sent via post two days ago and
> hopefully should reach FSF in one week.
>          Maybe it's OK to commit this patch now?
Not really.  It needs to be accepted by the FSF before we can include 
the work.

>
>       2. I am not turning member loop_depth of struct equivalence into
> short integer as GCC API such as bb_loop_depth
>           returns a loop's depth as a 32-bit interger.
There's already other places that assume loops don't nest that deep. 
Please go ahead and change it.  And no need to explicitly mark the 
unused bits.  That's just a maintenance nightmare in the long term 
anyway :-)


>
>       3. I find it's kind of difficult to use the new type and
> interfaces for list walking the init_insns list for this patch.
>          The type of init_insns list is rtx, not rtl_insn_list *. Seems
> we need to change a lot in order to use the new interface.
>          Not clear about the reason why it is not adjusted when we are
> transferring to the new interface.
>          Anyway, I think it's better to have another patch fix that issue. OK?
The right way to go is to add a checked cast when we have some code that 
is using the old interface and other code using the new interface.  It's 
actually a pretty easy change.

The checked casts effectively mark the limits of where we've been able 
to push the RTL typesafety work.  Long term as we push the typesafety 
work further into the compiler many/most of the checked casts will go away.

Unfortunately, that won't work in this case because other code wants to 
store a (const0_rtx) into the insn list.  (const0_rtx) isn't an INSN, so 
the checked cast fails and we get a nice abort/ICE.

Conceptually we just need another marker that is an INSN and we might as 
well just convert the whole file to use the new interface at that point.

Consider the request pulled.

The const0-rtx problem may be why this wasn't converted in the first 
palce.  Or it may simply have been a time problem.  David's done > 250 
patches around RTL typesafety, but he also has other work to be doing ;-)

>
>       4. This bug is only reproduceable with my local customized GCC
> version. So I don't have a testcase then.
OK.

I'll do a final review when I get notice about the copyright assignment 
from the FSF.

jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-26 21:03                 ` Jeff Law
@ 2014-09-27 14:48                   ` Felix Yang
  2014-09-29 21:44                     ` Jeff Law
  0 siblings, 1 reply; 27+ messages in thread
From: Felix Yang @ 2014-09-27 14:48 UTC (permalink / raw)
  To: Jeff Law; +Cc: Yangfei (Felix), GCC Patches, vmakarov

[-- Attachment #1: Type: text/plain, Size: 4633 bytes --]

Thanks for the explaination.
I have changed the loop_depth into a short interger hoping that we can
save some memory :-)
Attached please find the updated patch. Bootstrapped and reg-tested on
x86_64-suse-linux.
Please do a final revew once the assignment is ready.

As for the new list walking interface, I choose the function
"no_equiv" and tried the "checked cast" way.
The bad news is that GCC failed to bootstrap with the following change:

Index: ira.c
===================================================================
--- ira.c (revision 215536)
+++ ira.c (working copy)
@@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   void *data ATTRIBUTE_UNUSED)
 {
   int regno;
-  rtx list;
+  rtx_insn_list *list;

   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
-  list = reg_equiv[regno].init_insns;
+  list = as_a <rtx_insn_list *> (reg_equiv[regno].init_insns);
   if (list == const0_rtx)
     return;
   reg_equiv[regno].init_insns = const0_rtx;
@@ -3258,9 +3258,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
     return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = list->next ())
     {
-      rtx insn = XEXP (list, 0);
+      rtx_insn *insn = list->insn ();
       remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
     }
 }

Error message:
.... ...
checking for suffix of object files... configure: error: in
`/home/yangfei/gcc-devel/build/x86_64-unknown-linux-gnu/libgcc':
configure: error: cannot compute suffix of object files: cannot compile
See `config.log' for more details.
make[2]: *** [configure-stage1-target-libgcc] Error 1
make[2]: Leaving directory `/home/yangfei/gcc-devel/build'
make[1]: *** [stage1-bubble] Error 2
make[1]: Leaving directory `/home/yangfei/gcc-devel/build'
make: *** [all] Error 2

I think the code change is OK. Anything I missed?

Cheers,
Felix


On Sat, Sep 27, 2014 at 5:03 AM, Jeff Law <law@redhat.com> wrote:
> On 09/26/14 07:57, Felix Yang wrote:
>>
>> Hi Jeff,
>>
>>      Thanks for the suggestions. I updated the patch accordingly.
>>
>>      1. Both my employer(Huawei) and I have signed the copyright
>> assignments with FSF.
>>          These assignments are already sent via post two days ago and
>> hopefully should reach FSF in one week.
>>          Maybe it's OK to commit this patch now?
>
> Not really.  It needs to be accepted by the FSF before we can include the
> work.
>
>>
>>       2. I am not turning member loop_depth of struct equivalence into
>> short integer as GCC API such as bb_loop_depth
>>           returns a loop's depth as a 32-bit interger.
>
> There's already other places that assume loops don't nest that deep. Please
> go ahead and change it.  And no need to explicitly mark the unused bits.
> That's just a maintenance nightmare in the long term anyway :-)
>
>
>>
>>       3. I find it's kind of difficult to use the new type and
>> interfaces for list walking the init_insns list for this patch.
>>          The type of init_insns list is rtx, not rtl_insn_list *. Seems
>> we need to change a lot in order to use the new interface.
>>          Not clear about the reason why it is not adjusted when we are
>> transferring to the new interface.
>>          Anyway, I think it's better to have another patch fix that issue.
>> OK?
>
> The right way to go is to add a checked cast when we have some code that is
> using the old interface and other code using the new interface.  It's
> actually a pretty easy change.
>
> The checked casts effectively mark the limits of where we've been able to
> push the RTL typesafety work.  Long term as we push the typesafety work
> further into the compiler many/most of the checked casts will go away.
>
> Unfortunately, that won't work in this case because other code wants to
> store a (const0_rtx) into the insn list.  (const0_rtx) isn't an INSN, so the
> checked cast fails and we get a nice abort/ICE.
>
> Conceptually we just need another marker that is an INSN and we might as
> well just convert the whole file to use the new interface at that point.
>
> Consider the request pulled.
>
> The const0-rtx problem may be why this wasn't converted in the first palce.
> Or it may simply have been a time problem.  David's done > 250 patches
> around RTL typesafety, but he also has other work to be doing ;-)
>
>>
>>       4. This bug is only reproduceable with my local customized GCC
>> version. So I don't have a testcase then.
>
> OK.
>
> I'll do a final review when I get notice about the copyright assignment from
> the FSF.
>
> jeff
>

[-- Attachment #2: ira-update-equiv-regs-r3.diff --]
[-- Type: text/plain, Size: 5235 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 215658)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,14 @@
+2014-09-26  Felix Yang  <felix.yang@huawei.com>
+	    Jeff Law  <law@redhat.com>
+
+	* ira.c (struct equivalence): Change member "is_arg_equivalence" and "replace"
+	into boolean bitfields; turn member "loop_depth" into a short integer; add new
+	member "no_equiv" and "reserved".
+	(no_equiv): Set no_equiv of struct equivalence if register is marked
+	as having no known equivalence.
+	(update_equiv_regs): Check all definitions for a multiple-set
+	register to make sure that the RHS have the same value.
+
 2014-09-26  Jan Hubicka  <hubicka@ucw.cz>	
 	
 	PR ipa/60665
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 215658)
+++ gcc/ira.c	(working copy)
@@ -2894,12 +2894,14 @@ struct equivalence
   rtx init_insns;
   /* Loop depth is used to recognize equivalences which appear
      to be present within the same loop (or in an inner loop).  */
-  int loop_depth;
+  short loop_depth;
   /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
   /* Set when an attempt should be made to replace a register
      with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
 };
 
 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list == const0_rtx)
     return;
@@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
     return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = XEXP (list, 1))
     {
       rtx insn = XEXP (list, 0);
       remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
@@ -3373,7 +3376,7 @@ update_equiv_regs (void)
 
 	  /* If this insn contains more (or less) than a single SET,
 	     only mark all destinations as having no known equivalence.  */
-	  if (set == 0)
+	  if (set == NULL_RTX)
 	    {
 	      note_stores (PATTERN (insn), no_equiv, NULL);
 	      continue;
@@ -3467,16 +3470,48 @@ update_equiv_regs (void)
 	  if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 	    note = NULL_RTX;
 
-	  if (DF_REG_DEF_COUNT (regno) != 1
-	      && (! note
+	  if (DF_REG_DEF_COUNT (regno) != 1)
+	    {
+	      rtx list;
+	      bool equal_p = true;
+
+	      /* If we have already processed this pseudo and determined it
+		 can not have an equivalence, then honor that decision.  */
+	      if (reg_equiv[regno].no_equiv)
+		continue;
+
+	      if (! note
 		  || rtx_varies_p (XEXP (note, 0), 0)
 		  || (reg_equiv[regno].replacement
 		      && ! rtx_equal_p (XEXP (note, 0),
-					reg_equiv[regno].replacement))))
-	    {
-	      no_equiv (dest, set, NULL);
-	      continue;
+					reg_equiv[regno].replacement)))
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
+
+	      list = reg_equiv[regno].init_insns;
+	      for (; list; list = XEXP (list, 1))
+		{
+		  rtx note_tmp, insn_tmp;
+
+		  insn_tmp = XEXP (list, 0);
+		  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+		  gcc_assert (note_tmp);
+		  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+		    {
+		      equal_p = false;
+		      break;
+		    }
+		}
+
+	      if (! equal_p)
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
 	    }
+
 	  /* Record this insn as initializing this register.  */
 	  reg_equiv[regno].init_insns
 	    = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3505,10 +3540,9 @@ update_equiv_regs (void)
 	     a register used only in one basic block from a MEM.  If so, and the
 	     MEM remains unchanged for the life of the register, add a REG_EQUIV
 	     note.  */
-
 	  note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
 
-	  if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+	  if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
 	      && MEM_P (SET_SRC (set))
 	      && validate_equiv_mem (insn, dest, SET_SRC (set)))
 	    note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
@@ -3538,7 +3572,7 @@ update_equiv_regs (void)
 
 	      reg_equiv[regno].replacement = x;
 	      reg_equiv[regno].src_p = &SET_SRC (set);
-	      reg_equiv[regno].loop_depth = loop_depth;
+	      reg_equiv[regno].loop_depth = (short) loop_depth;
 
 	      /* Don't mess with things live during setjmp.  */
 	      if (REG_LIVE_LENGTH (regno) >= 0 && optimize)
@@ -3668,7 +3702,7 @@ update_equiv_regs (void)
 		  rtx equiv_insn;
 
 		  if (! reg_equiv[regno].replace
-		      || reg_equiv[regno].loop_depth < loop_depth
+		      || reg_equiv[regno].loop_depth < (short) loop_depth
 		      /* There is no sense to move insns if live range
 			 shrinkage or register pressure-sensitive
 			 scheduling were done because it will not

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-27 14:48                   ` Felix Yang
@ 2014-09-29 21:44                     ` Jeff Law
  2014-10-11 13:13                       ` Felix Yang
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Law @ 2014-09-29 21:44 UTC (permalink / raw)
  To: Felix Yang; +Cc: Yangfei (Felix), GCC Patches, vmakarov

On 09/27/14 08:48, Felix Yang wrote:
> Thanks for the explaination.
> I have changed the loop_depth into a short interger hoping that we can
> save some memory :-)
Thanks.

> Attached please find the updated patch. Bootstrapped and reg-tested on
> x86_64-suse-linux.
> Please do a final revew once the assignment is ready.
>
> As for the new list walking interface, I choose the function
> "no_equiv" and tried the "checked cast" way.
> The bad news is that GCC failed to bootstrap with the following change:
>
> Index: ira.c
> ===================================================================
> --- ira.c (revision 215536)
> +++ ira.c (working copy)
> @@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>     void *data ATTRIBUTE_UNUSED)
>   {
>     int regno;
> -  rtx list;
> +  rtx_insn_list *list;
>
>     if (!REG_P (reg))
>       return;
>     regno = REGNO (reg);
> -  list = reg_equiv[regno].init_insns;
> +  list = as_a <rtx_insn_list *> (reg_equiv[regno].init_insns);
>     if (list == const0_rtx)
>       return;
>     reg_equiv[regno].init_insns = const0_rtx;
> @@ -3258,9 +3258,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>       return;
>     ira_reg_equiv[regno].defined_p = false;
>     ira_reg_equiv[regno].init_insns = NULL;
> -  for (; list; list =  XEXP (list, 1))
> +  for (; list; list = list->next ())
>       {
> -      rtx insn = XEXP (list, 0);
> +      rtx_insn *insn = list->insn ();
>         remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
>       }
>   }
Yea.  I'm going to post a patch shortly to go ahead with this 
conversion.  There's a couple issues that come into play.

First const0_rtx is not an INSN, so we *really* don't want it in the 
INSN field of an INSN_LIST.  That's probably the ICE you're seeing.

const0_rtx is being used to mark pseudos which we've already determined 
can't have a valid equivalence.  So we just need a different marker. 
That different marker must be embeddable in an INSN_LIST node.   The 
easiest is just a NULL insn ;-)

The other tests for the const0_rtx marker in ira.c need relatively 
trivial updating.  And in the end we don't need the checked cast at all ;-)



Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-09-29 21:44                     ` Jeff Law
@ 2014-10-11 13:13                       ` Felix Yang
  2014-10-13 20:30                         ` Jeff Law
  2015-02-02 15:59                         ` Alex Velenko
  0 siblings, 2 replies; 27+ messages in thread
From: Felix Yang @ 2014-10-11 13:13 UTC (permalink / raw)
  To: Jeff Law; +Cc: Yangfei (Felix), GCC Patches, vmakarov

[-- Attachment #1: Type: text/plain, Size: 7902 bytes --]

Hello Jeff,

    I see that you have improved the RTL typesafety issue for ira.c,
so I rebased this patch
    on the latest trunk and change to use the new list walking interface.
    Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
    OK for trunk?

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog    (revision 216116)
+++ gcc/ChangeLog    (working copy)
@@ -1,3 +1,14 @@
+2014-10-11  Felix Yang  <felix.yang@huawei.com>
+        Jeff Law  <law@redhat.com>
+
+    * ira.c (struct equivalence): Change member "is_arg_equivalence"
and "replace"
+    into boolean bitfields; turn member "loop_depth" into a short
integer; add new
+    member "no_equiv" and "reserved".
+    (no_equiv): Set no_equiv of struct equivalence if register is marked
+    as having no known equivalence.
+    (update_equiv_regs): Check all definitions for a multiple-set
+    register to make sure that the RHS have the same value.
+
 2014-10-11  Martin Liska  <mliska@suse.cz>

     PR/63376
Index: gcc/ira.c
===================================================================
--- gcc/ira.c    (revision 216116)
+++ gcc/ira.c    (working copy)
@@ -2902,12 +2902,14 @@ struct equivalence

   /* Loop depth is used to recognize equivalences which appear
      to be present within the same loop (or in an inner loop).  */
-  int loop_depth;
+  short loop_depth;
   /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
   /* Set when an attempt should be made to replace a register
      with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
 };

 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3255,6 +3257,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list && list->insn () == NULL)
     return;
@@ -3381,7 +3384,7 @@ update_equiv_regs (void)

       /* If this insn contains more (or less) than a single SET,
          only mark all destinations as having no known equivalence.  */
-      if (set == 0)
+      if (set == NULL_RTX)
         {
           note_stores (PATTERN (insn), no_equiv, NULL);
           continue;
@@ -3476,16 +3479,49 @@ update_equiv_regs (void)
       if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
         note = NULL_RTX;

-      if (DF_REG_DEF_COUNT (regno) != 1
-          && (! note
+      if (DF_REG_DEF_COUNT (regno) != 1)
+        {
+          bool equal_p = true;
+          rtx_insn_list *list;
+
+          /* If we have already processed this pseudo and determined it
+         can not have an equivalence, then honor that decision.  */
+          if (reg_equiv[regno].no_equiv)
+        continue;
+
+          if (! note
           || rtx_varies_p (XEXP (note, 0), 0)
           || (reg_equiv[regno].replacement
               && ! rtx_equal_p (XEXP (note, 0),
-                    reg_equiv[regno].replacement))))
-        {
-          no_equiv (dest, set, NULL);
-          continue;
+                    reg_equiv[regno].replacement)))
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
+
+          list = reg_equiv[regno].init_insns;
+          for (; list; list = list->next ())
+        {
+          rtx note_tmp;
+          rtx_insn *insn_tmp;
+
+          insn_tmp = list->insn ();
+          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+          gcc_assert (note_tmp);
+          if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+            {
+              equal_p = false;
+              break;
+            }
+        }
+
+          if (! equal_p)
+        {
+          no_equiv (dest, set, NULL);
+          continue;
+        }
         }
+
       /* Record this insn as initializing this register.  */
       reg_equiv[regno].init_insns
         = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3514,10 +3550,9 @@ update_equiv_regs (void)
          a register used only in one basic block from a MEM.  If so, and the
          MEM remains unchanged for the life of the register, add a REG_EQUIV
          note.  */
-
       note = find_reg_note (insn, REG_EQUIV, NULL_RTX);

-      if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+      if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
           && MEM_P (SET_SRC (set))
           && validate_equiv_mem (insn, dest, SET_SRC (set)))
         note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
@@ -3547,7 +3582,7 @@ update_equiv_regs (void)

           reg_equiv[regno].replacement = x;
           reg_equiv[regno].src_p = &SET_SRC (set);
-          reg_equiv[regno].loop_depth = loop_depth;
+          reg_equiv[regno].loop_depth = (short) loop_depth;

           /* Don't mess with things live during setjmp.  */
           if (REG_LIVE_LENGTH (regno) >= 0 && optimize)
@@ -3677,7 +3712,7 @@ update_equiv_regs (void)
           rtx equiv_insn;

           if (! reg_equiv[regno].replace
-              || reg_equiv[regno].loop_depth < loop_depth
+              || reg_equiv[regno].loop_depth < (short) loop_depth
               /* There is no sense to move insns if live range
              shrinkage or register pressure-sensitive
              scheduling were done because it will not
Cheers,
Felix


On Tue, Sep 30, 2014 at 5:44 AM, Jeff Law <law@redhat.com> wrote:
> On 09/27/14 08:48, Felix Yang wrote:
>>
>> Thanks for the explaination.
>> I have changed the loop_depth into a short interger hoping that we can
>> save some memory :-)
>
> Thanks.
>
>
>> Attached please find the updated patch. Bootstrapped and reg-tested on
>> x86_64-suse-linux.
>> Please do a final revew once the assignment is ready.
>>
>> As for the new list walking interface, I choose the function
>> "no_equiv" and tried the "checked cast" way.
>> The bad news is that GCC failed to bootstrap with the following change:
>>
>> Index: ira.c
>> ===================================================================
>> --- ira.c (revision 215536)
>> +++ ira.c (working copy)
>> @@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>>     void *data ATTRIBUTE_UNUSED)
>>   {
>>     int regno;
>> -  rtx list;
>> +  rtx_insn_list *list;
>>
>>     if (!REG_P (reg))
>>       return;
>>     regno = REGNO (reg);
>> -  list = reg_equiv[regno].init_insns;
>> +  list = as_a <rtx_insn_list *> (reg_equiv[regno].init_insns);
>>     if (list == const0_rtx)
>>       return;
>>     reg_equiv[regno].init_insns = const0_rtx;
>> @@ -3258,9 +3258,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>>       return;
>>     ira_reg_equiv[regno].defined_p = false;
>>     ira_reg_equiv[regno].init_insns = NULL;
>> -  for (; list; list =  XEXP (list, 1))
>> +  for (; list; list = list->next ())
>>       {
>> -      rtx insn = XEXP (list, 0);
>> +      rtx_insn *insn = list->insn ();
>>         remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
>>       }
>>   }
>
> Yea.  I'm going to post a patch shortly to go ahead with this conversion.
> There's a couple issues that come into play.
>
> First const0_rtx is not an INSN, so we *really* don't want it in the INSN
> field of an INSN_LIST.  That's probably the ICE you're seeing.
>
> const0_rtx is being used to mark pseudos which we've already determined
> can't have a valid equivalence.  So we just need a different marker. That
> different marker must be embeddable in an INSN_LIST node.   The easiest is
> just a NULL insn ;-)
>
> The other tests for the const0_rtx marker in ira.c need relatively trivial
> updating.  And in the end we don't need the checked cast at all ;-)
>
>
>
> Jeff

[-- Attachment #2: ira-update-equiv-regs-r4.diff --]
[-- Type: text/plain, Size: 4886 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 216116)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,14 @@
+2014-10-11  Felix Yang  <felix.yang@huawei.com>
+	    Jeff Law  <law@redhat.com>
+
+	* ira.c (struct equivalence): Change member "is_arg_equivalence" and "replace"
+	into boolean bitfields; turn member "loop_depth" into a short integer; add new
+	member "no_equiv" and "reserved".
+	(no_equiv): Set no_equiv of struct equivalence if register is marked
+	as having no known equivalence.
+	(update_equiv_regs): Check all definitions for a multiple-set
+	register to make sure that the RHS have the same value.
+
 2014-10-11  Martin Liska  <mliska@suse.cz>
 
 	PR/63376
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 216116)
+++ gcc/ira.c	(working copy)
@@ -2902,12 +2902,14 @@ struct equivalence
 
   /* Loop depth is used to recognize equivalences which appear
      to be present within the same loop (or in an inner loop).  */
-  int loop_depth;
+  short loop_depth;
   /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
   /* Set when an attempt should be made to replace a register
      with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
 };
 
 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3255,6 +3257,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
     return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list && list->insn () == NULL)
     return;
@@ -3381,7 +3384,7 @@ update_equiv_regs (void)
 
 	  /* If this insn contains more (or less) than a single SET,
 	     only mark all destinations as having no known equivalence.  */
-	  if (set == 0)
+	  if (set == NULL_RTX)
 	    {
 	      note_stores (PATTERN (insn), no_equiv, NULL);
 	      continue;
@@ -3476,16 +3479,49 @@ update_equiv_regs (void)
 	  if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 	    note = NULL_RTX;
 
-	  if (DF_REG_DEF_COUNT (regno) != 1
-	      && (! note
+	  if (DF_REG_DEF_COUNT (regno) != 1)
+	    {
+	      bool equal_p = true;
+	      rtx_insn_list *list;
+
+	      /* If we have already processed this pseudo and determined it
+		 can not have an equivalence, then honor that decision.  */
+	      if (reg_equiv[regno].no_equiv)
+		continue;
+
+	      if (! note
 		  || rtx_varies_p (XEXP (note, 0), 0)
 		  || (reg_equiv[regno].replacement
 		      && ! rtx_equal_p (XEXP (note, 0),
-					reg_equiv[regno].replacement))))
-	    {
-	      no_equiv (dest, set, NULL);
-	      continue;
+					reg_equiv[regno].replacement)))
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
+
+	      list = reg_equiv[regno].init_insns;
+	      for (; list; list = list->next ())
+		{
+		  rtx note_tmp;
+		  rtx_insn *insn_tmp;
+
+		  insn_tmp = list->insn ();
+		  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+		  gcc_assert (note_tmp);
+		  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+		    {
+		      equal_p = false;
+		      break;
+		    }
+		}
+
+	      if (! equal_p)
+		{
+		  no_equiv (dest, set, NULL);
+		  continue;
+		}
 	    }
+
 	  /* Record this insn as initializing this register.  */
 	  reg_equiv[regno].init_insns
 	    = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3514,10 +3550,9 @@ update_equiv_regs (void)
 	     a register used only in one basic block from a MEM.  If so, and the
 	     MEM remains unchanged for the life of the register, add a REG_EQUIV
 	     note.  */
-
 	  note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
 
-	  if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+	  if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
 	      && MEM_P (SET_SRC (set))
 	      && validate_equiv_mem (insn, dest, SET_SRC (set)))
 	    note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
@@ -3547,7 +3582,7 @@ update_equiv_regs (void)
 
 	      reg_equiv[regno].replacement = x;
 	      reg_equiv[regno].src_p = &SET_SRC (set);
-	      reg_equiv[regno].loop_depth = loop_depth;
+	      reg_equiv[regno].loop_depth = (short) loop_depth;
 
 	      /* Don't mess with things live during setjmp.  */
 	      if (REG_LIVE_LENGTH (regno) >= 0 && optimize)
@@ -3677,7 +3712,7 @@ update_equiv_regs (void)
 		  rtx equiv_insn;
 
 		  if (! reg_equiv[regno].replace
-		      || reg_equiv[regno].loop_depth < loop_depth
+		      || reg_equiv[regno].loop_depth < (short) loop_depth
 		      /* There is no sense to move insns if live range
 			 shrinkage or register pressure-sensitive
 			 scheduling were done because it will not

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-10-11 13:13                       ` Felix Yang
@ 2014-10-13 20:30                         ` Jeff Law
  2015-02-02 15:59                         ` Alex Velenko
  1 sibling, 0 replies; 27+ messages in thread
From: Jeff Law @ 2014-10-13 20:30 UTC (permalink / raw)
  To: Felix Yang; +Cc: Yangfei (Felix), GCC Patches, vmakarov

On 10/11/14 06:44, Felix Yang wrote:
> Hello Jeff,
>
>      I see that you have improved the RTL typesafety issue for ira.c,
> so I rebased this patch
>      on the latest trunk and change to use the new list walking interface.
>      Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
>      OK for trunk?
OK for the trunk.

Thanks for your patience.

jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2014-10-11 13:13                       ` Felix Yang
  2014-10-13 20:30                         ` Jeff Law
@ 2015-02-02 15:59                         ` Alex Velenko
  2015-02-03  7:24                           ` Jeff Law
  1 sibling, 1 reply; 27+ messages in thread
From: Alex Velenko @ 2015-02-02 15:59 UTC (permalink / raw)
  To: Felix Yang, Jeff Law
  Cc: Yangfei (Felix), Marcus.Shawcroft, GCC Patches, vmakarov

On 11/10/14 13:44, Felix Yang wrote:
> Hello Jeff,
>
>      I see that you have improved the RTL typesafety issue for ira.c,
> so I rebased this patch
>      on the latest trunk and change to use the new list walking interface.
>      Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
>      OK for trunk?
Hi Felix,
I believe your patch causes a regression for arm-none-eabi.
FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2

This happens because your patch stops reuse of code for
" return -1;" statements in pr43920-2.c.

As far as I investigated, your patch prevents adding "(expr_list (-1) 
(nil)" in ira pass, which prevents jump2 optimization from happening.

So before, in ira pass I could see:
"(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
         (const_int -1 [0xffffffffffffffff])) 
/work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 613 
{*thumb2_movsi_vfp}
      (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
         (nil)))"
But with your patch I get
"(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
         (const_int -1 [0xffffffffffffffff])) 
/work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 
615 {*thumb2_movsi_vfp}
      (nil))"

This causes a code generation regression and needs to be fixed.
Kind regards,
Alex

>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog    (revision 216116)
> +++ gcc/ChangeLog    (working copy)
> @@ -1,3 +1,14 @@
> +2014-10-11  Felix Yang  <felix.yang@huawei.com>
> +        Jeff Law  <law@redhat.com>
> +
> +    * ira.c (struct equivalence): Change member "is_arg_equivalence"
> and "replace"
> +    into boolean bitfields; turn member "loop_depth" into a short
> integer; add new
> +    member "no_equiv" and "reserved".
> +    (no_equiv): Set no_equiv of struct equivalence if register is marked
> +    as having no known equivalence.
> +    (update_equiv_regs): Check all definitions for a multiple-set
> +    register to make sure that the RHS have the same value.
> +
>   2014-10-11  Martin Liska  <mliska@suse.cz>
>
>       PR/63376
> Index: gcc/ira.c
> ===================================================================
> --- gcc/ira.c    (revision 216116)
> +++ gcc/ira.c    (working copy)
> @@ -2902,12 +2902,14 @@ struct equivalence
>
>     /* Loop depth is used to recognize equivalences which appear
>        to be present within the same loop (or in an inner loop).  */
> -  int loop_depth;
> +  short loop_depth;
>     /* Nonzero if this had a preexisting REG_EQUIV note.  */
> -  int is_arg_equivalence;
> +  unsigned char is_arg_equivalence : 1;
>     /* Set when an attempt should be made to replace a register
>        with the associated src_p entry.  */
> -  char replace;
> +  unsigned char replace : 1;
> +  /* Set if this register has no known equivalence.  */
> +  unsigned char no_equiv : 1;
>   };
>
>   /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
> @@ -3255,6 +3257,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>     if (!REG_P (reg))
>       return;
>     regno = REGNO (reg);
> +  reg_equiv[regno].no_equiv = 1;
>     list = reg_equiv[regno].init_insns;
>     if (list && list->insn () == NULL)
>       return;
> @@ -3381,7 +3384,7 @@ update_equiv_regs (void)
>
>         /* If this insn contains more (or less) than a single SET,
>            only mark all destinations as having no known equivalence.  */
> -      if (set == 0)
> +      if (set == NULL_RTX)
>           {
>             note_stores (PATTERN (insn), no_equiv, NULL);
>             continue;
> @@ -3476,16 +3479,49 @@ update_equiv_regs (void)
>         if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>           note = NULL_RTX;
>
> -      if (DF_REG_DEF_COUNT (regno) != 1
> -          && (! note
> +      if (DF_REG_DEF_COUNT (regno) != 1)
> +        {
> +          bool equal_p = true;
> +          rtx_insn_list *list;
> +
> +          /* If we have already processed this pseudo and determined it
> +         can not have an equivalence, then honor that decision.  */
> +          if (reg_equiv[regno].no_equiv)
> +        continue;
> +
> +          if (! note
>             || rtx_varies_p (XEXP (note, 0), 0)
>             || (reg_equiv[regno].replacement
>                 && ! rtx_equal_p (XEXP (note, 0),
> -                    reg_equiv[regno].replacement))))
> -        {
> -          no_equiv (dest, set, NULL);
> -          continue;
> +                    reg_equiv[regno].replacement)))
> +        {
> +          no_equiv (dest, set, NULL);
> +          continue;
> +        }
> +
> +          list = reg_equiv[regno].init_insns;
> +          for (; list; list = list->next ())
> +        {
> +          rtx note_tmp;
> +          rtx_insn *insn_tmp;
> +
> +          insn_tmp = list->insn ();
> +          note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
> +          gcc_assert (note_tmp);
> +          if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
> +            {
> +              equal_p = false;
> +              break;
> +            }
> +        }
> +
> +          if (! equal_p)
> +        {
> +          no_equiv (dest, set, NULL);
> +          continue;
> +        }
>           }
> +
>         /* Record this insn as initializing this register.  */
>         reg_equiv[regno].init_insns
>           = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
> @@ -3514,10 +3550,9 @@ update_equiv_regs (void)
>            a register used only in one basic block from a MEM.  If so, and the
>            MEM remains unchanged for the life of the register, add a REG_EQUIV
>            note.  */
> -
>         note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
>
> -      if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
> +      if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
>             && MEM_P (SET_SRC (set))
>             && validate_equiv_mem (insn, dest, SET_SRC (set)))
>           note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
> @@ -3547,7 +3582,7 @@ update_equiv_regs (void)
>
>             reg_equiv[regno].replacement = x;
>             reg_equiv[regno].src_p = &SET_SRC (set);
> -          reg_equiv[regno].loop_depth = loop_depth;
> +          reg_equiv[regno].loop_depth = (short) loop_depth;
>
>             /* Don't mess with things live during setjmp.  */
>             if (REG_LIVE_LENGTH (regno) >= 0 && optimize)
> @@ -3677,7 +3712,7 @@ update_equiv_regs (void)
>             rtx equiv_insn;
>
>             if (! reg_equiv[regno].replace
> -              || reg_equiv[regno].loop_depth < loop_depth
> +              || reg_equiv[regno].loop_depth < (short) loop_depth
>                 /* There is no sense to move insns if live range
>                shrinkage or register pressure-sensitive
>                scheduling were done because it will not
> Cheers,
> Felix
>
>
> On Tue, Sep 30, 2014 at 5:44 AM, Jeff Law <law@redhat.com> wrote:
>> On 09/27/14 08:48, Felix Yang wrote:
>>>
>>> Thanks for the explaination.
>>> I have changed the loop_depth into a short interger hoping that we can
>>> save some memory :-)
>>
>> Thanks.
>>
>>
>>> Attached please find the updated patch. Bootstrapped and reg-tested on
>>> x86_64-suse-linux.
>>> Please do a final revew once the assignment is ready.
>>>
>>> As for the new list walking interface, I choose the function
>>> "no_equiv" and tried the "checked cast" way.
>>> The bad news is that GCC failed to bootstrap with the following change:
>>>
>>> Index: ira.c
>>> ===================================================================
>>> --- ira.c (revision 215536)
>>> +++ ira.c (working copy)
>>> @@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>>>      void *data ATTRIBUTE_UNUSED)
>>>    {
>>>      int regno;
>>> -  rtx list;
>>> +  rtx_insn_list *list;
>>>
>>>      if (!REG_P (reg))
>>>        return;
>>>      regno = REGNO (reg);
>>> -  list = reg_equiv[regno].init_insns;
>>> +  list = as_a <rtx_insn_list *> (reg_equiv[regno].init_insns);
>>>      if (list == const0_rtx)
>>>        return;
>>>      reg_equiv[regno].init_insns = const0_rtx;
>>> @@ -3258,9 +3258,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>>>        return;
>>>      ira_reg_equiv[regno].defined_p = false;
>>>      ira_reg_equiv[regno].init_insns = NULL;
>>> -  for (; list; list =  XEXP (list, 1))
>>> +  for (; list; list = list->next ())
>>>        {
>>> -      rtx insn = XEXP (list, 0);
>>> +      rtx_insn *insn = list->insn ();
>>>          remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
>>>        }
>>>    }
>>
>> Yea.  I'm going to post a patch shortly to go ahead with this conversion.
>> There's a couple issues that come into play.
>>
>> First const0_rtx is not an INSN, so we *really* don't want it in the INSN
>> field of an INSN_LIST.  That's probably the ICE you're seeing.
>>
>> const0_rtx is being used to mark pseudos which we've already determined
>> can't have a valid equivalence.  So we just need a different marker. That
>> different marker must be embeddable in an INSN_LIST node.   The easiest is
>> just a NULL insn ;-)
>>
>> The other tests for the const0_rtx marker in ira.c need relatively trivial
>> updating.  And in the end we don't need the checked cast at all ;-)
>>
>>
>>
>> Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-02 15:59                         ` Alex Velenko
@ 2015-02-03  7:24                           ` Jeff Law
  2015-02-03  8:29                             ` Bin.Cheng
  0 siblings, 1 reply; 27+ messages in thread
From: Jeff Law @ 2015-02-03  7:24 UTC (permalink / raw)
  To: Alex Velenko, Felix Yang
  Cc: Yangfei (Felix), Marcus.Shawcroft, GCC Patches, vmakarov

On 02/02/15 08:59, Alex Velenko wrote:
> On 11/10/14 13:44, Felix Yang wrote:
>> Hello Jeff,
>>
>>      I see that you have improved the RTL typesafety issue for ira.c,
>> so I rebased this patch
>>      on the latest trunk and change to use the new list walking
>> interface.
>>      Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
>>      OK for trunk?
> Hi Felix,
> I believe your patch causes a regression for arm-none-eabi.
> FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
> FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2
>
> This happens because your patch stops reuse of code for
> " return -1;" statements in pr43920-2.c.
>
> As far as I investigated, your patch prevents adding "(expr_list (-1)
> (nil)" in ira pass, which prevents jump2 optimization from happening.
>
> So before, in ira pass I could see:
> "(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
>          (const_int -1 [0xffffffffffffffff]))
> /work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 613
> {*thumb2_movsi_vfp}
>       (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
>          (nil)))"
> But with your patch I get
> "(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
>          (const_int -1 [0xffffffffffffffff]))
> /work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
> 615 {*thumb2_movsi_vfp}
>       (nil))"
>
> This causes a code generation regression and needs to be fixed.
> Kind regards,
We'd need to see the full dumps.  In particular is reg110 set anywhere 
else?  If so then the change is doing precisely what it should be doing 
and the test needs to be updated to handle the different code we generate.

Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-03  7:24                           ` Jeff Law
@ 2015-02-03  8:29                             ` Bin.Cheng
  2015-02-03 12:42                               ` Alex Velenko
  2015-02-03 16:28                               ` Jeff Law
  0 siblings, 2 replies; 27+ messages in thread
From: Bin.Cheng @ 2015-02-03  8:29 UTC (permalink / raw)
  To: Jeff Law
  Cc: Alex Velenko, Felix Yang, Yangfei (Felix),
	Marcus Shawcroft, GCC Patches, vmakarov

On Tue, Feb 3, 2015 at 3:24 PM, Jeff Law <law@redhat.com> wrote:
> On 02/02/15 08:59, Alex Velenko wrote:
>>
>> On 11/10/14 13:44, Felix Yang wrote:
>>>
>>> Hello Jeff,
>>>
>>>      I see that you have improved the RTL typesafety issue for ira.c,
>>> so I rebased this patch
>>>      on the latest trunk and change to use the new list walking
>>> interface.
>>>      Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
>>>      OK for trunk?
>>
>> Hi Felix,
>> I believe your patch causes a regression for arm-none-eabi.
>> FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
>> FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2
>>
>> This happens because your patch stops reuse of code for
>> " return -1;" statements in pr43920-2.c.
>>
>> As far as I investigated, your patch prevents adding "(expr_list (-1)
>> (nil)" in ira pass, which prevents jump2 optimization from happening.
>>
>> So before, in ira pass I could see:
>> "(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
>>          (const_int -1 [0xffffffffffffffff]))
>> /work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
>> 613
>> {*thumb2_movsi_vfp}
>>       (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
>>          (nil)))"
>> But with your patch I get
>> "(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
>>          (const_int -1 [0xffffffffffffffff]))
>> /work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
>> 615 {*thumb2_movsi_vfp}
>>       (nil))"
>>
>> This causes a code generation regression and needs to be fixed.
>> Kind regards,
>
> We'd need to see the full dumps.  In particular is reg110 set anywhere else?
> If so then the change is doing precisely what it should be doing and the
> test needs to be updated to handle the different code we generate.

Hmm, if I understand correctly, it's a code size regression, so I
don't think it's appropriate to adapt the test case.  Either the patch
or something else in GCC is doing wrong, right?

Hi Alex, could you please file a PR with full dump information for tracking?

Thanks,
bin
>
> Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-03  8:29                             ` Bin.Cheng
@ 2015-02-03 12:42                               ` Alex Velenko
  2015-02-03 16:28                               ` Jeff Law
  1 sibling, 0 replies; 27+ messages in thread
From: Alex Velenko @ 2015-02-03 12:42 UTC (permalink / raw)
  To: Bin.Cheng, Jeff Law
  Cc: Felix Yang, Yangfei (Felix), Marcus Shawcroft, GCC Patches, vmakarov

On 03/02/15 08:29, Bin.Cheng wrote:
> On Tue, Feb 3, 2015 at 3:24 PM, Jeff Law <law@redhat.com> wrote:
>> On 02/02/15 08:59, Alex Velenko wrote:
>>>
>>> On 11/10/14 13:44, Felix Yang wrote:
>>>>
>>>> Hello Jeff,
>>>>
>>>>       I see that you have improved the RTL typesafety issue for ira.c,
>>>> so I rebased this patch
>>>>       on the latest trunk and change to use the new list walking
>>>> interface.
>>>>       Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
>>>>       OK for trunk?
>>>
>>> Hi Felix,
>>> I believe your patch causes a regression for arm-none-eabi.
>>> FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
>>> FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2
>>>
>>> This happens because your patch stops reuse of code for
>>> " return -1;" statements in pr43920-2.c.
>>>
>>> As far as I investigated, your patch prevents adding "(expr_list (-1)
>>> (nil)" in ira pass, which prevents jump2 optimization from happening.
>>>
>>> So before, in ira pass I could see:
>>> "(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
>>>           (const_int -1 [0xffffffffffffffff]))
>>> /work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
>>> 613
>>> {*thumb2_movsi_vfp}
>>>        (expr_list:REG_EQUAL (const_int -1 [0xffffffffffffffff])
>>>           (nil)))"
>>> But with your patch I get
>>> "(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
>>>           (const_int -1 [0xffffffffffffffff]))
>>> /work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
>>> 615 {*thumb2_movsi_vfp}
>>>        (nil))"
>>>
>>> This causes a code generation regression and needs to be fixed.
>>> Kind regards,
>>
>> We'd need to see the full dumps.  In particular is reg110 set anywhere else?
>> If so then the change is doing precisely what it should be doing and the
>> test needs to be updated to handle the different code we generate.
>
> Hmm, if I understand correctly, it's a code size regression, so I
> don't think it's appropriate to adapt the test case.  Either the patch
> or something else in GCC is doing wrong, right?
>
> Hi Alex, could you please file a PR with full dump information for tracking?
>
> Thanks,
> bin
Hi Bin,
Created bugzilla ticket, as requested:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64916
This test already existed in the testsuite, it is not new.
Kind regards,
Alex

>>
>> Jeff
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-03  8:29                             ` Bin.Cheng
  2015-02-03 12:42                               ` Alex Velenko
@ 2015-02-03 16:28                               ` Jeff Law
  2015-02-04  3:03                                 ` Bin.Cheng
  1 sibling, 1 reply; 27+ messages in thread
From: Jeff Law @ 2015-02-03 16:28 UTC (permalink / raw)
  To: Bin.Cheng
  Cc: Alex Velenko, Felix Yang, Yangfei (Felix),
	Marcus Shawcroft, GCC Patches, vmakarov

On 02/03/15 01:29, Bin.Cheng wrote:
>
> Hmm, if I understand correctly, it's a code size regression, so I
> don't think it's appropriate to adapt the test case.  Either the patch
> or something else in GCC is doing wrong, right?
>
> Hi Alex, could you please file a PR with full dump information for tracking?
But if the code size regression is due to the older compiler incorrectly 
handling the promotion of REG_EQUAL to REG_EQUIV notes, then the test 
absolutely does need updating as the codesize was dependent on incorrect 
behaviour in the compiler.

jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-03 16:28                               ` Jeff Law
@ 2015-02-04  3:03                                 ` Bin.Cheng
  2015-02-09 23:32                                   ` Jeff Law
  0 siblings, 1 reply; 27+ messages in thread
From: Bin.Cheng @ 2015-02-04  3:03 UTC (permalink / raw)
  To: Jeff Law
  Cc: Alex Velenko, Felix Yang, Yangfei (Felix),
	Marcus Shawcroft, GCC Patches, vmakarov

On Wed, Feb 4, 2015 at 12:28 AM, Jeff Law <law@redhat.com> wrote:
> On 02/03/15 01:29, Bin.Cheng wrote:
>>
>>
>> Hmm, if I understand correctly, it's a code size regression, so I
>> don't think it's appropriate to adapt the test case.  Either the patch
>> or something else in GCC is doing wrong, right?
>>
>> Hi Alex, could you please file a PR with full dump information for
>> tracking?
>
> But if the code size regression is due to the older compiler incorrectly
> handling the promotion of REG_EQUAL to REG_EQUIV notes, then the test
> absolutely does need updating as the codesize was dependent on incorrect
> behaviour in the compiler.

Hi Jeff,

I looked into the test and can confirm the previous compilation is correct.
The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
but in this case it is REG_EQUAL that is lost.  The full dump (without
this patch) after IRA is like:

   10: NOTE_INSN_BASIC_BLOCK 2
    2: r116:SI=r0:SI
    3: r117:SI=r1:SI
      REG_DEAD r1:SI
    4: r118:SI=r2:SI
      REG_DEAD r2:SI
    5: NOTE_INSN_FUNCTION_BEG
   12: r2:SI=0x1
   13: r1:SI=0
   15: r0:SI=call [`lseek'] argc:0
      REG_DEAD r2:SI
      REG_DEAD r1:SI
      REG_CALL_DECL `lseek'
   16: r111:SI=r0:SI
      REG_DEAD r0:SI
   17: r2:SI=0x2
   18: r1:SI=0
   19: r0:SI=r116:SI
      REG_DEAD r116:SI
   20: r0:SI=call [`lseek'] argc:0
      REG_DEAD r2:SI
      REG_DEAD r1:SI
      REG_CALL_DECL `lseek'
   21: r112:SI=r0:SI
      REG_DEAD r0:SI
   22: cc:CC=cmp(r111:SI,0xffffffffffffffff)
   23: pc={(cc:CC==0)?L46:pc}
      REG_DEAD cc:CC
      REG_BR_PROB 159
   24: NOTE_INSN_BASIC_BLOCK 3
   25: cc:CC=cmp(r112:SI,0xffffffffffffffff)
   26: pc={(cc:CC==0)?L50:pc}
      REG_DEAD cc:CC
      REG_BR_PROB 159
   27: NOTE_INSN_BASIC_BLOCK 4
   28: NOTE_INSN_DELETED
   29: {cc:CC_NOOV=cmp(r112:SI-r111:SI,0);r114:SI=r112:SI-r111:SI;}
      REG_DEAD r112:SI
   30: pc={(cc:CC_NOOV==0)?L54:pc}
      REG_DEAD cc:CC_NOOV
      REG_BR_PROB 400
   31: NOTE_INSN_BASIC_BLOCK 5
   32: [r117:SI]=r111:SI
      REG_DEAD r117:SI
      REG_DEAD r111:SI
   33: [r118:SI]=r114:SI
      REG_DEAD r118:SI
      REG_DEAD r114:SI
    7: r110:SI=0
      REG_EQUAL 0
   76: pc=L34
   77: barrier
   46: L46:
   45: NOTE_INSN_BASIC_BLOCK 6
    8: r110:SI=r111:SI
      REG_DEAD r111:SI
      REG_EQUAL 0xffffffffffffffff
   78: pc=L34
   79: barrier
   50: L50:
   49: NOTE_INSN_BASIC_BLOCK 7
    6: r110:SI=r112:SI
      REG_DEAD r112:SI
      REG_EQUAL 0xffffffffffffffff
   80: pc=L34
   81: barrier
   54: L54:
   53: NOTE_INSN_BASIC_BLOCK 8
    9: r110:SI=0xffffffffffffffff
      REG_EQUAL 0xffffffffffffffff
   34: L34:
   35: NOTE_INSN_BASIC_BLOCK 9
   40: r0:SI=r110:SI
      REG_DEAD r110:SI
   41: use r0:SI

Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
-> 9 can be merged because r110 equals to -1 afterwards.  But with the
patch, the equal information of r110==-1 in basic block 8 is lost.  As
a result, jump from 8->9 can't be merged and two additional
instructions are generated.

I suppose the REG_EQUAL note is correct in insn9?  According to
GCCint, it only means r110 set by insn9 will be equal to the value at
run time at the end of this insn but not necessarily elsewhere in the
function.

I also found another problem (or mis-leading?) with the document:
"Thus, compiler passes prior to register allocation need only check
for REG_EQUAL notes and passes subsequent to register allocation need
only check for REG_EQUIV notes".  This seems not true now as in this
example, passes after register allocation do take advantage of
REG_EQUAL in optimization and we can't achieve that by using
REG_EQUIV.

Thanks,
bin

>
> jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-04  3:03                                 ` Bin.Cheng
@ 2015-02-09 23:32                                   ` Jeff Law
  2015-02-10 10:51                                     ` Ajit Kumar Agarwal
  2015-02-12 12:40                                     ` Alex Velenko
  0 siblings, 2 replies; 27+ messages in thread
From: Jeff Law @ 2015-02-09 23:32 UTC (permalink / raw)
  To: Bin.Cheng
  Cc: Alex Velenko, Felix Yang, Yangfei (Felix),
	Marcus Shawcroft, GCC Patches, vmakarov

On 02/03/15 20:03, Bin.Cheng wrote:
> I looked into the test and can confirm the previous compilation is correct.
> The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
> but in this case it is REG_EQUAL that is lost.  The full dump (without
> this patch) after IRA is like:
Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL 
notes in the insn stream.  Basically update_equiv_regs will scan insn 
stream and some REG_EQUAL notes will be promoted to REG_EQUIV notes.

The REG_EQUIV is a function-wide equivalence, meaning that one could 
substitute the REG_EQUIV note for in any uses of the destination 
register and still have a valid representation of the program.

REG_EQUAL's validity is limited to the point after the insn in which it 
appears and before the next insn.

>
> Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
> -> 9 can be merged because r110 equals to -1 afterwards.  But with the
> patch, the equal information of r110==-1 in basic block 8 is lost.  As
> a result, jump from 8->9 can't be merged and two additional
> instructions are generated.
 >
> I suppose the REG_EQUAL note is correct in insn9?  According to
> GCCint, it only means r110 set by insn9 will be equal to the value at
> run time at the end of this insn but not necessarily elsewhere in the
> function.
If you previously got a REG_EQUIV note on any of those insns it was 
wrong and this is the precise kind of situation that the change was 
trying to fix.

R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no 
single value across the entire function that one can validly use for r110.

I think you could mark this as a missed optimization, but not a 
regresssion since the desired output was relying on a bug in the compiler.

If I were to start looking at this, my first thought would be to look at 
why we have multiple sets of r110, particularly if there are lifetimes 
that are disjoint.


>
> I also found another problem (or mis-leading?) with the document:
> "Thus, compiler passes prior to register allocation need only check
> for REG_EQUAL notes and passes subsequent to register allocation need
> only check for REG_EQUIV notes".  This seems not true now as in this
> example, passes after register allocation do take advantage of
> REG_EQUAL in optimization and we can't achieve that by using
> REG_EQUIV.
I think that's a long standing (and incorrect) generalization.  IIRC we 
can get a REG_EQUIV note earlier for certain argument setup situations. 
  And I think it's been the case for a long time that a pass after 
reload could try to exploit REG_EQUAL notes.

jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-09 23:32                                   ` Jeff Law
@ 2015-02-10 10:51                                     ` Ajit Kumar Agarwal
  2015-02-10 15:46                                       ` Jeff Law
  2015-02-12 12:40                                     ` Alex Velenko
  1 sibling, 1 reply; 27+ messages in thread
From: Ajit Kumar Agarwal @ 2015-02-10 10:51 UTC (permalink / raw)
  To: Jeff Law, Bin.Cheng
  Cc: Alex Velenko, Felix Yang, Yangfei (Felix),
	Marcus Shawcroft, GCC Patches, vmakarov



-----Original Message-----
From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] On Behalf Of Jeff Law
Sent: Tuesday, February 10, 2015 5:03 AM
To: Bin.Cheng
Cc: Alex Velenko; Felix Yang; Yangfei (Felix); Marcus Shawcroft; GCC Patches; vmakarov@redhat.com
Subject: Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

On 02/03/15 20:03, Bin.Cheng wrote:
> I looked into the test and can confirm the previous compilation is correct.
> The cover letter of this patch said IRA mis-handled REQ_EQUIV before, 
> but in this case it is REG_EQUAL that is lost.  The full dump (without 
> this patch) after IRA is like:
Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL notes in the insn stream.  Basically update_equiv_regs will scan insn stream and some REG_EQUAL notes will be promoted to REG_EQUIV notes.

The REG_EQUIV is a function-wide equivalence, meaning that one could substitute the REG_EQUIV note for in any uses of the destination register and still have a valid representation of the program.

REG_EQUAL's validity is limited to the point after the insn in which it appears and before the next insn.

>
> Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
> -> 9 can be merged because r110 equals to -1 afterwards.  But with the
> patch, the equal information of r110==-1 in basic block 8 is lost.  As 
> a result, jump from 8->9 can't be merged and two additional 
> instructions are generated.
 >
> I suppose the REG_EQUAL note is correct in insn9?  According to 
> GCCint, it only means r110 set by insn9 will be equal to the value at 
> run time at the end of this insn but not necessarily elsewhere in the 
> function.
>>If you previously got a REG_EQUIV note on any of those insns it was wrong and this is the precise kind of situation that the change was trying to fix.

>>R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no single value across the entire function that one can validly use for r110.

Does the value of R110 should not change across all the callee path from the given caller functions.  If the value is aliased, then how the call side affects should make
sure the value remains same across all the callee chain path from  the given caller function. I am curious to know how the value remain constant throughout the function
 is identified in case of aliasing and the interprocedural case.

>>I think you could mark this as a missed optimization, but not a regresssion since the desired output was relying on a bug in the compiler.
>>
>>If I were to start looking at this, my first thought would be to look at why we have multiple sets of r110, particularly if there are lifetimes that are disjoint.


>
> I also found another problem (or mis-leading?) with the document:
> "Thus, compiler passes prior to register allocation need only check 
> for REG_EQUAL notes and passes subsequent to register allocation need 
> only check for REG_EQUIV notes".  This seems not true now as in this 
> example, passes after register allocation do take advantage of 
> REG_EQUAL in optimization and we can't achieve that by using 
> REG_EQUIV.
>>I think that's a long standing (and incorrect) generalization.  IIRC we can get a REG_EQUIV note earlier for certain argument setup situations. 
  >>And I think it's been the case for a long time that a pass after reload could try to exploit REG_EQUAL notes.

Thanks & Regards
Ajit
jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-10 10:51                                     ` Ajit Kumar Agarwal
@ 2015-02-10 15:46                                       ` Jeff Law
  0 siblings, 0 replies; 27+ messages in thread
From: Jeff Law @ 2015-02-10 15:46 UTC (permalink / raw)
  To: Ajit Kumar Agarwal, Bin.Cheng
  Cc: Alex Velenko, Felix Yang, Yangfei (Felix),
	Marcus Shawcroft, GCC Patches, vmakarov

On 02/10/15 03:51, Ajit Kumar Agarwal wrote:
>>> R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no single value across the entire function that one can validly use for r110.
>
> Does the value of R110 should not change across all the callee path from the given caller functions.  If the value is aliased, then how the call side affects should make
> sure the value remains same across all the callee chain path from  the given caller function. I am curious to know how the value remain constant throughout the function
>   is identified in case of aliasing and the interprocedural case.
Pseudo registers are allowed to have aliases.


Jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-09 23:32                                   ` Jeff Law
  2015-02-10 10:51                                     ` Ajit Kumar Agarwal
@ 2015-02-12 12:40                                     ` Alex Velenko
  2015-02-13 23:18                                       ` Jeff Law
  1 sibling, 1 reply; 27+ messages in thread
From: Alex Velenko @ 2015-02-12 12:40 UTC (permalink / raw)
  To: Jeff Law, Bin.Cheng
  Cc: Felix Yang, Yangfei (Felix), Marcus Shawcroft, GCC Patches, vmakarov

On 09/02/15 23:32, Jeff Law wrote:
> On 02/03/15 20:03, Bin.Cheng wrote:
>> I looked into the test and can confirm the previous compilation is correct.
>> The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
>> but in this case it is REG_EQUAL that is lost.  The full dump (without
>> this patch) after IRA is like:
> Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL
> notes in the insn stream.  Basically update_equiv_regs will scan insn
> stream and some REG_EQUAL notes will be promoted to REG_EQUIV notes.

Hi Jeff,
Do I understand you correctly, that REG_EQUAL notes should not be
generated in IRA pass, because some of them may get promoted to
REG_EQUIV? Or is there any other reason register r110 should not get
REG_EQUAL note? Previously, register r110 was getting REG_EQUAL, not
REG_EQUIV note.
Kind regards,
Alex

>
> The REG_EQUIV is a function-wide equivalence, meaning that one could
> substitute the REG_EQUIV note for in any uses of the destination
> register and still have a valid representation of the program.
>
> REG_EQUAL's validity is limited to the point after the insn in which it
> appears and before the next insn.
>
>>
>> Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
>> -> 9 can be merged because r110 equals to -1 afterwards.  But with the
>> patch, the equal information of r110==-1 in basic block 8 is lost.  As
>> a result, jump from 8->9 can't be merged and two additional
>> instructions are generated.
>   >
>> I suppose the REG_EQUAL note is correct in insn9?  According to
>> GCCint, it only means r110 set by insn9 will be equal to the value at
>> run time at the end of this insn but not necessarily elsewhere in the
>> function.
> If you previously got a REG_EQUIV note on any of those insns it was
> wrong and this is the precise kind of situation that the change was
> trying to fix.
>
> R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no
> single value across the entire function that one can validly use for r110.
>
> I think you could mark this as a missed optimization, but not a
> regresssion since the desired output was relying on a bug in the compiler.
>
> If I were to start looking at this, my first thought would be to look at
> why we have multiple sets of r110, particularly if there are lifetimes
> that are disjoint.
>
>
>>
>> I also found another problem (or mis-leading?) with the document:
>> "Thus, compiler passes prior to register allocation need only check
>> for REG_EQUAL notes and passes subsequent to register allocation need
>> only check for REG_EQUIV notes".  This seems not true now as in this
>> example, passes after register allocation do take advantage of
>> REG_EQUAL in optimization and we can't achieve that by using
>> REG_EQUIV.
> I think that's a long standing (and incorrect) generalization.  IIRC we
> can get a REG_EQUIV note earlier for certain argument setup situations.
>    And I think it's been the case for a long time that a pass after
> reload could try to exploit REG_EQUAL notes.
>
> jeff
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
  2015-02-12 12:40                                     ` Alex Velenko
@ 2015-02-13 23:18                                       ` Jeff Law
  0 siblings, 0 replies; 27+ messages in thread
From: Jeff Law @ 2015-02-13 23:18 UTC (permalink / raw)
  To: Alex Velenko, Bin.Cheng
  Cc: Felix Yang, Yangfei (Felix), Marcus Shawcroft, GCC Patches, vmakarov

On 02/12/15 05:40, Alex Velenko wrote:
> On 09/02/15 23:32, Jeff Law wrote:
>> On 02/03/15 20:03, Bin.Cheng wrote:
>>> I looked into the test and can confirm the previous compilation is
>>> correct.
>>> The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
>>> but in this case it is REG_EQUAL that is lost.  The full dump (without
>>> this patch) after IRA is like:
>> Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL
>> notes in the insn stream.  Basically update_equiv_regs will scan insn
>> stream and some REG_EQUAL notes will be promoted to REG_EQUIV notes.
>
> Hi Jeff,
> Do I understand you correctly, that REG_EQUAL notes should not be
> generated in IRA pass, because some of them may get promoted to
> REG_EQUIV? Or is there any other reason register r110 should not get
> REG_EQUAL note? Previously, register r110 was getting REG_EQUAL, not
> REG_EQUIV note.
The reason r110 can not get a REG_EQUIV note is because there are 
multiple insns that set r110 to different values.

The equivalency created by a REG_EQUIV note has to be valid at every 
point within the function.  Thus for any pseudo that holds > 1 distinct 
value within a function there can be no valid REG_EQUIV notes on the 
assignments to that pseudo.



jeff

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2015-02-13 23:18 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-22 14:40 [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition Felix Yang
2014-09-22 18:40 ` Jeff Law
2014-09-23  2:48   ` Yangfei (Felix)
2014-09-23 10:46     ` Felix Yang
2014-09-23 10:51       ` Felix Yang
2014-09-23 17:49         ` Jeff Law
2014-09-24 12:07           ` Felix Yang
2014-09-24 21:56             ` Felix Yang
2014-09-25  4:07               ` [PING PATCH " Yangfei (Felix)
2014-09-25 18:57             ` [PATCH " Jeff Law
2014-09-26 13:57               ` Felix Yang
2014-09-26 21:03                 ` Jeff Law
2014-09-27 14:48                   ` Felix Yang
2014-09-29 21:44                     ` Jeff Law
2014-10-11 13:13                       ` Felix Yang
2014-10-13 20:30                         ` Jeff Law
2015-02-02 15:59                         ` Alex Velenko
2015-02-03  7:24                           ` Jeff Law
2015-02-03  8:29                             ` Bin.Cheng
2015-02-03 12:42                               ` Alex Velenko
2015-02-03 16:28                               ` Jeff Law
2015-02-04  3:03                                 ` Bin.Cheng
2015-02-09 23:32                                   ` Jeff Law
2015-02-10 10:51                                     ` Ajit Kumar Agarwal
2015-02-10 15:46                                       ` Jeff Law
2015-02-12 12:40                                     ` Alex Velenko
2015-02-13 23:18                                       ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).