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

[Felix Yang <fei dot yang0953 at gmail dot com>]

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: ira-update-equiv-regs-new.diff
Description: Text document