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