diff mbox

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

Message ID CAFc0fxyBTaQOah3OBZUgs2UzQmn3RQzN4+Fm2Jou3V3=OYS4pw@mail.gmail.com
State New
Headers show

Commit Message

Felix Yang Sept. 26, 2014, 1:57 p.m. UTC 
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

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

Comments

Jeff Law Sept. 26, 2014, 9:03 p.m. UTC | #1 
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
diff mbox

Patch

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