diff mbox

[rtl] PR middle-end/78016, keep REG_NOTE order during insn copy

Message ID 1f9dd2ba-6a92-3afc-a6fb-d4be914ab239@foss.arm.com
State New
Headers show

Commit Message

Jiong Wang Oct. 20, 2016, 3:28 p.m. UTC 
As discussed on PR middle-end/78016, here is the patch.

This patch makes EXPR_LIST/INST_LIST/INT_LIST insertion bi-directional, the new
node can be inserted to either the start or the end of the given list.

The existed alloc_EXPR_LIST, alloc_INSN_LIST becomes wrapper of new
bi-directional function, there is no functional change on them, callers of them
are *not affected*.

This patch then factor out those REG_NOTES copy code in emit-rtl.c and
sel-sched-ir.c into a function append_insn_reg_notes in rtlanal.c, it use those
new bi-directional interfaces to make sure the order of REG_NOTES are not
changed during insn copy.  Redundant code in emit-rtl.c and sel-sched-ir.c are
deleted also.

x86_64/aarch64 bootstrap OK. c/c++ regression OK.

OK for trunk?

gcc/
2016-10-20  Jiong Wang  <jiong.wang@arm.com>

         PR middle-end/78016
         * lists.c (alloc_INSN_LIST_bidirection): New function.  The function
         body is cloned from alloc_INSN_LIST with minor changes to make it
         support bi-directional insertion.
         (alloc_EXPR_LIST_bidirection): Likewise.
         (alloc_INT_LIST_bidirection): New function.  Alloc INT_LIST node, and
         support bi-directional insertion into given list.
         (alloc_INSN_LIST): Call alloc_INSN_LIST_bidirection.
         (alloc_EXPR_LIST): Call alloc_EXPR_LIST_bidirection.
         * rtl.h (append_insn_reg_notes): New declaration.
         (alloc_INSN_LIST_bidirection): New declaration.
         (alloc_EXPR_LIST_bidirection): New declaration.
         (alloc_INT_LIST_bidirection): New declaration.
         * rtlanal.c (alloc_reg_note_bidirection): New static function.  Function
         body is cloned from alloc_reg_note with minor changes to make it support
         bi-directional insertion.
         (alloc_reg_note): Call alloc_reg_note_bidirection.
         (append_insn_reg_notes): New function.
         * emit-rtl.c (emit_copy_of_insn_after): Use append_insn_reg_notes.
         * sel-sched-ir.c (create_copy_of_insn_rtx): Likewise.

Comments

Bernd Schmidt Oct. 20, 2016, 3:41 p.m. UTC | #1 
On 10/20/2016 05:28 PM, Jiong Wang wrote:
> This patch makes EXPR_LIST/INST_LIST/INT_LIST insertion bi-directional,

> the new

> node can be inserted to either the start or the end of the given list.


I can't help but feel that this is somewhat more complicated than it 
needs to be.

One thing to note is that we had a recent discussion about boolean 
parameters and how they don't exactly always make for a good interface.

> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c

> index 2d6d1eb..87eb1e3 100644

> --- a/gcc/emit-rtl.c

> +++ b/gcc/emit-rtl.c

> @@ -6125,7 +6125,6 @@ rtx_insn *

>  emit_copy_of_insn_after (rtx_insn *insn, rtx_insn *after)

>  {

>    rtx_insn *new_rtx;

> -  rtx link;

>

>    switch (GET_CODE (insn))

>      {

> @@ -6171,15 +6170,7 @@ emit_copy_of_insn_after (rtx_insn *insn, rtx_insn *after)

>    /* Copy all REG_NOTES except REG_LABEL_OPERAND since mark_jump_label

>       will make them.  REG_LABEL_TARGETs are created there too, but are

>       supposed to be sticky, so we copy them.  */

> -  for (link = REG_NOTES (insn); link; link = XEXP (link, 1))

> -    if (REG_NOTE_KIND (link) != REG_LABEL_OPERAND)

> -      {

> -	if (GET_CODE (link) == EXPR_LIST)

> -	  add_reg_note (new_rtx, REG_NOTE_KIND (link),

> -			copy_insn_1 (XEXP (link, 0)));

> -	else

> -	  add_shallow_copy_of_reg_note (new_rtx, link);

> -      }

> +  append_insn_reg_notes (new_rtx, insn, true, false);


Looks like two such loops got replaced with one much more heavyweight 
function. I'd prefer to keep the loops here and just keep a pointer to 
the tail. I think you wouldn't need any of the new functions in this 
case and the patch would be much smaller.

>  /* This call is used in place of a gen_rtx_INSN_LIST. If there is a cached

>     node available, we'll use it, otherwise a call to gen_rtx_INSN_LIST

> -   is made.  */

> +   is made.  The new node will be appended at the end of LIST if APPEND_P is

> +   TRUE, otherwise list is appended to the new node.  */

> +

>  rtx_insn_list *

> -alloc_INSN_LIST (rtx val, rtx next)

> +alloc_INSN_LIST_bidirection (rtx val, rtx list, bool append_p)

>  {

>    rtx_insn_list *r;

> +  rtx next = append_p ? NULL_RTX : list;

>

>    if (unused_insn_list)

>      {

> @@ -117,16 +120,33 @@ alloc_INSN_LIST (rtx val, rtx next)

>    else

>      r = gen_rtx_INSN_LIST (VOIDmode, val, next);

>

> +  if (append_p)

> +    {

> +      gcc_assert (list != NULL_RTX);

> +      XEXP (list, 1) = r;

> +    }

> +

>    return r;

>  }


Looks like you could keep the original function as-is and add a 
alloc_INSN_LIST_append that passes NULL_RTX for list. Also, this looks 
like you're not appending to the end of a list, but replacing everything 
that follows the first element.

> +/* Append all REG_NOTES in order from FROM_INSN to end of existed REG_NOTES of

> +   TO_INSN.  Skip REG_LABEL_OPERAND if skip_reg_label_p is TRUE, skip REG_EQUAL

> +   and REG_EQUIV if skip_reg_equ_p is TRUE.  */


See earlier note about boolean parameters...

> +void

> +append_insn_reg_notes (rtx_insn *to_insn, rtx_insn *from_insn,

> +		       bool skip_reg_label_p, bool skip_reg_equ_p)

> +{

> +  /* Locate to the end of existed REG_NOTES of TO_INSN.  */

> +  rtx tail = REG_NOTES (to_insn);

> +

> +  if (tail != NULL_RTX)

> +    {

> +      rtx tail_old;

> +

> +      do {

> +	  tail_old = tail;

> +	  tail = XEXP (tail, 1);

> +      } while (tail != NULL_RTX);

> +

> +      tail = tail_old;

> +    }


That's also overcomplicated.

rtx *ptail = &REG_NOTES (to_insn);
while (*ptail != NULL_RTX)
   ptail = &XEXP (*ptail, 1);

gives you a pointer to the end which you can then use to append, 
unconditionally. As mentioned above, I think it would be simpler to keep 
this logic in the caller functions and avoid introducing 
append_insn_reg_notes.


Bernd
Eric Botcazou Oct. 21, 2016, 7:43 a.m. UTC | #2 
> That's also overcomplicated.


Yes, I agree that's too heavy.

> rtx *ptail = &REG_NOTES (to_insn);

> while (*ptail != NULL_RTX)

>    ptail = &XEXP (*ptail, 1);

> 

> gives you a pointer to the end which you can then use to append,

> unconditionally. As mentioned above, I think it would be simpler to keep

> this logic in the caller functions and avoid introducing

> append_insn_reg_notes.


I disagree: there are currently n ways of copying NOTEs in the RTL middle-end, 
with different properties each time.  We need only one primitive in rtlanal.c.

-- 
Eric Botcazou
Jiong Wang Oct. 21, 2016, 9:38 a.m. UTC | #3 
On 21/10/16 08:43, Eric Botcazou wrote:
>> That's also overcomplicated.

> Yes, I agree that's too heavy.

>

>> rtx *ptail = &REG_NOTES (to_insn);

>> while (*ptail != NULL_RTX)

>>     ptail = &XEXP (*ptail, 1);


Thanks very much Bernd, yes, this is better.  And through manipulating 
pointer directly, those bidirectional new functions are unnecessary.

>>

>> gives you a pointer to the end which you can then use to append,

>> unconditionally. As mentioned above, I think it would be simpler to keep

>> this logic in the caller functions and avoid introducing

>> append_insn_reg_notes.

> I disagree: there are currently n ways of copying NOTEs in the RTL middle-end,

> with different properties each time.  We need only one primitive in rtlanal.c.

That's my view,  those duplicated code in emit-rtl.c and sel-sched-ir.c 
really can be shared and append all REG_NOTES from one insn to another 
seems qualify one primitive in rtlanal.c

I will come up with a patch much lighter.

Thanks.
Bernd Schmidt Oct. 21, 2016, 10:13 a.m. UTC | #4 
On 10/21/2016 09:43 AM, Eric Botcazou wrote:
> I disagree: there are currently n ways of copying NOTEs in the RTL middle-end,

> with different properties each time.  We need only one primitive in rtlanal.c.


I feel the fact that they have different properties means we shouldn't 
try to unify them: we'll just end up with a long list of boolean 
parameters, with no way of quickly telling what a given function call is 
doing. A copy loop is short enough that it can be implemented in-place 
and people can quickly tell what is going on by looking at it.

Maybe the inner if statement could be a small helper function 
(append_copy_of_reg_note).


Bernd
diff mbox

Patch

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 2d6d1eb..87eb1e3 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -6125,7 +6125,6 @@  rtx_insn *
 emit_copy_of_insn_after (rtx_insn *insn, rtx_insn *after)
 {
   rtx_insn *new_rtx;
-  rtx link;
 
   switch (GET_CODE (insn))
     {
@@ -6171,15 +6170,7 @@  emit_copy_of_insn_after (rtx_insn *insn, rtx_insn *after)
   /* Copy all REG_NOTES except REG_LABEL_OPERAND since mark_jump_label
      will make them.  REG_LABEL_TARGETs are created there too, but are
      supposed to be sticky, so we copy them.  */
-  for (link = REG_NOTES (insn); link; link = XEXP (link, 1))
-    if (REG_NOTE_KIND (link) != REG_LABEL_OPERAND)
-      {
-	if (GET_CODE (link) == EXPR_LIST)
-	  add_reg_note (new_rtx, REG_NOTE_KIND (link),
-			copy_insn_1 (XEXP (link, 0)));
-	else
-	  add_shallow_copy_of_reg_note (new_rtx, link);
-      }
+  append_insn_reg_notes (new_rtx, insn, true, false);
 
   INSN_CODE (new_rtx) = INSN_CODE (insn);
   return new_rtx;
diff --git a/gcc/lists.c b/gcc/lists.c
index 96b4bc7..cd30b7c 100644
--- a/gcc/lists.c
+++ b/gcc/lists.c
@@ -98,11 +98,14 @@  remove_list_elem (rtx elem, rtx *listp)
 
 /* This call is used in place of a gen_rtx_INSN_LIST. If there is a cached
    node available, we'll use it, otherwise a call to gen_rtx_INSN_LIST
-   is made.  */
+   is made.  The new node will be appended at the end of LIST if APPEND_P is
+   TRUE, otherwise list is appended to the new node.  */
+
 rtx_insn_list *
-alloc_INSN_LIST (rtx val, rtx next)
+alloc_INSN_LIST_bidirection (rtx val, rtx list, bool append_p)
 {
   rtx_insn_list *r;
+  rtx next = append_p ? NULL_RTX : list;
 
   if (unused_insn_list)
     {
@@ -117,16 +120,33 @@  alloc_INSN_LIST (rtx val, rtx next)
   else
     r = gen_rtx_INSN_LIST (VOIDmode, val, next);
 
+  if (append_p)
+    {
+      gcc_assert (list != NULL_RTX);
+      XEXP (list, 1) = r;
+    }
+
   return r;
 }
 
+/* Allocate new INSN_LIST node for VAL, append NEXT to it.  */
+
+rtx_insn_list *
+alloc_INSN_LIST (rtx val, rtx next)
+{
+  return alloc_INSN_LIST_bidirection (val, next, false);
+}
+
 /* This call is used in place of a gen_rtx_EXPR_LIST. If there is a cached
    node available, we'll use it, otherwise a call to gen_rtx_EXPR_LIST
-   is made.  */
+   is made.  The new node will be appended at the end of LIST if APPEND_P is
+   TRUE, otherwise list is appended to the new node.  */
+
 rtx_expr_list *
-alloc_EXPR_LIST (int kind, rtx val, rtx next)
+alloc_EXPR_LIST_bidirection (int kind, rtx val, rtx list, bool append_p)
 {
   rtx_expr_list *r;
+  rtx next = append_p ? NULL_RTX : list;
 
   if (unused_expr_list)
     {
@@ -139,9 +159,23 @@  alloc_EXPR_LIST (int kind, rtx val, rtx next)
   else
     r = gen_rtx_EXPR_LIST ((machine_mode) kind, val, next);
 
+  if (append_p)
+    {
+      gcc_assert (list != NULL_RTX);
+      XEXP (list, 1) = r;
+    }
+
   return r;
 }
 
+/* Allocate new EXPR_LIST node for KIND and VAL, append NEXT to it.  */
+
+rtx_expr_list *
+alloc_EXPR_LIST (int kind, rtx val, rtx next)
+{
+  return alloc_EXPR_LIST_bidirection (kind, val, next, false);
+}
+
 /* This function will free up an entire list of EXPR_LIST nodes.  */
 void
 free_EXPR_LIST_list (rtx_expr_list **listp)
@@ -242,4 +276,22 @@  remove_free_EXPR_LIST_node (rtx_expr_list **listp)
   return elem;
 }
 
+/* Allocate new INT_LIST node for KIND and VAL, append it to LIST if APPEND_P is
+   TRUE, otherwise append LIST to it.  */
+
+rtx
+alloc_INT_LIST_bidirection (int kind, int val, rtx list, bool append_p)
+{
+  rtx r = gen_rtx_INT_LIST ((machine_mode) kind, val,
+			    append_p ? NULL_RTX : list);
+
+  if (append_p)
+    {
+      gcc_assert (list != NULL_RTX);
+      XEXP (list, 1) = r;
+    }
+
+  return r;
+}
+
 #include "gt-lists.h"
diff --git a/gcc/rtl.h b/gcc/rtl.h
index ce1131b..5ce0c27 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2996,6 +2996,7 @@  extern rtx alloc_reg_note (enum reg_note, rtx, rtx);
 extern void add_reg_note (rtx, enum reg_note, rtx);
 extern void add_int_reg_note (rtx, enum reg_note, int);
 extern void add_shallow_copy_of_reg_note (rtx_insn *, rtx);
+extern void append_insn_reg_notes (rtx_insn *, rtx_insn *, bool, bool);
 extern void remove_note (rtx, const_rtx);
 extern void remove_reg_equal_equiv_notes (rtx_insn *);
 extern void remove_reg_equal_equiv_notes_for_regno (unsigned int);
@@ -3095,13 +3096,16 @@  extern void free_INSN_LIST_list (rtx_insn_list **);
 extern void free_EXPR_LIST_node (rtx);
 extern void free_INSN_LIST_node (rtx);
 extern rtx_insn_list *alloc_INSN_LIST (rtx, rtx);
+extern rtx_insn_list *alloc_INSN_LIST_bidirection (rtx, rtx, bool);
 extern rtx_insn_list *copy_INSN_LIST (rtx_insn_list *);
 extern rtx_insn_list *concat_INSN_LIST (rtx_insn_list *, rtx_insn_list *);
 extern rtx_expr_list *alloc_EXPR_LIST (int, rtx, rtx);
+extern rtx_expr_list *alloc_EXPR_LIST_bidirection (int, rtx, rtx, bool);
 extern void remove_free_INSN_LIST_elem (rtx_insn *, rtx_insn_list **);
 extern rtx remove_list_elem (rtx, rtx *);
 extern rtx_insn *remove_free_INSN_LIST_node (rtx_insn_list **);
 extern rtx remove_free_EXPR_LIST_node (rtx_expr_list **);
+extern rtx alloc_INT_LIST_bidirection (int, int, rtx, bool);
 
 
 /* reginfo.c */
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index 2a0a1d2..7effdd8 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -2244,10 +2244,12 @@  int_reg_note_p (enum reg_note kind)
 }
 
 /* Allocate a register note with kind KIND and datum DATUM.  LIST is
-   stored as the pointer to the next register note.  */
+   stored as the pointer to the existed register note.  Append new note to LIST
+   if APPEND_P is TRUE, otherwise append LIST to new note.  */
 
-rtx
-alloc_reg_note (enum reg_note kind, rtx datum, rtx list)
+static rtx
+alloc_reg_note_bidirection (enum reg_note kind, rtx datum, rtx list,
+			    bool append_p)
 {
   rtx note;
 
@@ -2262,18 +2264,27 @@  alloc_reg_note (enum reg_note kind, rtx datum, rtx list)
       /* These types of register notes use an INSN_LIST rather than an
 	 EXPR_LIST, so that copying is done right and dumps look
 	 better.  */
-      note = alloc_INSN_LIST (datum, list);
+      note = alloc_INSN_LIST_bidirection (datum, list, append_p);
       PUT_REG_NOTE_KIND (note, kind);
       break;
 
     default:
-      note = alloc_EXPR_LIST (kind, datum, list);
+      note = alloc_EXPR_LIST_bidirection (kind, datum, list, append_p);
       break;
     }
 
   return note;
 }
 
+/* Allocate a register note with kind KIND and datum DATUM.  Append LIST to the
+   new note.  */
+
+rtx
+alloc_reg_note (enum reg_note kind, rtx datum, rtx list)
+{
+  return alloc_reg_note_bidirection (kind, datum, list, false);
+}
+
 /* Add register note with kind KIND and datum DATUM to INSN.  */
 
 void
@@ -2334,6 +2345,66 @@  remove_note (rtx insn, const_rtx note)
     }
 }
 
+/* Append all REG_NOTES in order from FROM_INSN to end of existed REG_NOTES of
+   TO_INSN.  Skip REG_LABEL_OPERAND if skip_reg_label_p is TRUE, skip REG_EQUAL
+   and REG_EQUIV if skip_reg_equ_p is TRUE.  */
+
+void
+append_insn_reg_notes (rtx_insn *to_insn, rtx_insn *from_insn,
+		       bool skip_reg_label_p, bool skip_reg_equ_p)
+{
+  /* Locate to the end of existed REG_NOTES of TO_INSN.  */
+  rtx tail = REG_NOTES (to_insn);
+
+  if (tail != NULL_RTX)
+    {
+      rtx tail_old;
+
+      do {
+	  tail_old = tail;
+	  tail = XEXP (tail, 1);
+      } while (tail != NULL_RTX);
+
+      tail = tail_old;
+    }
+
+  /* Do the insertion.  */
+  rtx link;
+  enum reg_note kind;
+  for (link = REG_NOTES (from_insn); link; link = XEXP (link, 1))
+    {
+      kind = REG_NOTE_KIND (link);
+
+      if ((skip_reg_label_p && kind == REG_LABEL_OPERAND)
+	  || (skip_reg_equ_p && (kind == REG_EQUAL || kind == REG_EQUIV)))
+	continue;
+
+      if (tail == NULL_RTX)
+	{
+	  /* Initialize the head of REG_NOTES for TO_RTX if necessary.  */
+	  if (GET_CODE (link) == INT_LIST)
+	    add_shallow_copy_of_reg_note (to_insn, link);
+	  else
+	    add_reg_note (to_insn, kind, (GET_CODE (link) == EXPR_LIST
+					  ? copy_insn_1 (XEXP (link, 0))
+					  : XEXP (link ,0)));
+	  tail = REG_NOTES (to_insn);
+	}
+      else
+	{
+	  /* Append to the end of existed REG_NOTES of TO_RTX.  */
+	  if (GET_CODE (link) == INT_LIST)
+	    tail = alloc_INT_LIST_bidirection (kind, XINT (link, 0), tail,
+					       true);
+	  else
+	    tail = alloc_reg_note_bidirection (kind,
+					       (GET_CODE (link) == EXPR_LIST
+						? copy_insn_1 (XEXP (link, 0))
+						: XEXP (link, 0)), tail, true);
+	}
+    }
+}
+
 /* Remove REG_EQUAL and/or REG_EQUIV notes if INSN has such notes.  */
 
 void
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 210b1e4..bd28f15 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -5750,7 +5750,6 @@  rtx_insn *
 create_copy_of_insn_rtx (rtx insn_rtx)
 {
   rtx_insn *res;
-  rtx link;
 
   if (DEBUG_INSN_P (insn_rtx))
     return create_insn_rtx_from_pattern (copy_rtx (PATTERN (insn_rtx)),
@@ -5764,17 +5763,7 @@  create_copy_of_insn_rtx (rtx insn_rtx)
   /* Copy all REG_NOTES except REG_EQUAL/REG_EQUIV and REG_LABEL_OPERAND
      since mark_jump_label will make them.  REG_LABEL_TARGETs are created
      there too, but are supposed to be sticky, so we copy them.  */
-  for (link = REG_NOTES (insn_rtx); link; link = XEXP (link, 1))
-    if (REG_NOTE_KIND (link) != REG_LABEL_OPERAND
-	&& REG_NOTE_KIND (link) != REG_EQUAL
-	&& REG_NOTE_KIND (link) != REG_EQUIV)
-      {
-	if (GET_CODE (link) == EXPR_LIST)
-	  add_reg_note (res, REG_NOTE_KIND (link),
-			copy_insn_1 (XEXP (link, 0)));
-	else
-	  add_reg_note (res, REG_NOTE_KIND (link), XEXP (link, 0));
-      }
+  append_insn_reg_notes (res, safe_as_a <rtx_insn *> (insn_rtx), true, true);
 
   return res;
 }