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

[Jiong Wang]

On 07/11/16 17:04, Bernd Schmidt wrote:

On 11/03/2016 03:00 PM, Eric Botcazou wrote:

FWIW here's a more complete version of my patch which I'm currently
testing. Let me know if you think it's at least a good enough
intermediate step to be installed.


It is, thanks.


Testing showed the same issue as Jiong found, so I've committed it 
with that extra tweak.


Thanks very much!  I have closed PR middle-end/78016

Regards,
Jiong

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

[Bernd Schmidt]

On 11/03/2016 03:00 PM, Eric Botcazou wrote:

FWIW here's a more complete version of my patch which I'm currently
testing. Let me know if you think it's at least a good enough
intermediate step to be installed.


It is, thanks.


Testing showed the same issue as Jiong found, so I've committed it with 
that extra tweak.



Bernd

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

[Jiong Wang]

On 03/11/16 13:01, Bernd Schmidt wrote:


Index: gcc/emit-rtl.c
===
--- gcc/emit-rtl.c  (revision 241233)
+++ gcc/emit-rtl.c  (working copy)
@@ -6169,17 +6169,18 @@ emit_copy_of_insn_after (rtx_insn *insn,
   which may be duplicated by the basic block reordering code.  */
RTX_FRAME_RELATED_P (new_rtx) = RTX_FRAME_RELATED_P (insn);
  
+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */

+  rtx *ptail = _NOTES (new_rtx);
+  gcc_assert (*ptail == NULL_RTX);
+


Looks like new_rtx may contain it's own REG_NOTES when reached here thus
triggered ICE, I guess mark_jump_label may generate REG_LABEL_OPERAND as the
comments says.

After replace the gcc_assert with the following loop, this patch passed
bootstrap on both AArch64 and X86-64, and regression OK on gcc and g++.

+  while (*ptail != NULL_RTX)
+ptail =  (*ptail, 1);

Regards,
Jiong

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

[Eric Botcazou]

> FWIW here's a more complete version of my patch which I'm currently
> testing. Let me know if you think it's at least a good enough
> intermediate step to be installed.

It is, thanks.

> I think, the sel-sched example notwithstanding, this is something that
> normally should not be needed outside of emit_copy_of_insn_after, so having
> that do the right thing ought to be good enough.

reload does direct note copying too (in forward order).

-- 
Eric Botcazou

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

[Bernd Schmidt]

On 11/03/2016 01:33 PM, Eric Botcazou wrote:

Thanks for the feedback,  I'll try to work through this.


Thanks, but since there doesn't seem to be a consensus on the goal, feel free
to disregard it and just implement what you need for your initial work.


FWIW here's a more complete version of my patch which I'm currently 
testing. Let me know if you think it's at least a good enough 
intermediate step to be installed. I think, the sel-sched example 
notwithstanding, this is something that normally should not be needed 
outside of emit_copy_of_insn_after, so having that do the right thing 
ought to be good enough.



Bernd

	* emit-rtl.c (emit_copy_of_insn_after): Duplicate notes in order.
	* sel-sched-ir.c (create_copy_of_insn_rtx): Likewise.
	* rtl.h (duplicate_reg_notes): Declare.
	* rtlanal.c (duplicate_reg_note): New function.

Index: gcc/emit-rtl.c
===
--- gcc/emit-rtl.c	(revision 241233)
+++ gcc/emit-rtl.c	(working copy)
@@ -6169,17 +6169,18 @@ emit_copy_of_insn_after (rtx_insn *insn,
  which may be duplicated by the basic block reordering code.  */
   RTX_FRAME_RELATED_P (new_rtx) = RTX_FRAME_RELATED_P (insn);
 
+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */
+  rtx *ptail = _NOTES (new_rtx);
+  gcc_assert (*ptail == NULL_RTX);
+
   /* 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);
+	*ptail = duplicate_reg_note (link);
+	ptail =  (*ptail, 1);
   }
 
   INSN_CODE (new_rtx) = INSN_CODE (insn);
Index: gcc/rtl.h
===
--- gcc/rtl.h	(revision 241233)
+++ gcc/rtl.h	(working copy)
@@ -3008,6 +3008,7 @@ extern rtx alloc_reg_note (enum reg_note
 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 rtx duplicate_reg_note (rtx);
 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);
Index: gcc/rtlanal.c
===
--- gcc/rtlanal.c	(revision 241233)
+++ gcc/rtlanal.c	(working copy)
@@ -2304,6 +2304,20 @@ add_shallow_copy_of_reg_note (rtx_insn *
 add_reg_note (insn, REG_NOTE_KIND (note), XEXP (note, 0));
 }
 
+/* Duplicate NOTE and return the copy.  */
+rtx
+duplicate_reg_note (rtx note)
+{
+  reg_note kind = REG_NOTE_KIND (note);
+
+  if (GET_CODE (note) == INT_LIST)
+return gen_rtx_INT_LIST ((machine_mode) kind, XINT (note, 0), NULL_RTX);
+  else if (GET_CODE (note) == EXPR_LIST)
+return alloc_reg_note (kind, copy_insn_1 (XEXP (note, 0)), NULL_RTX);
+  else
+return alloc_reg_note (kind, XEXP (note, 0), NULL_RTX);
+}
+
 /* Remove register note NOTE from the REG_NOTES of INSN.  */
 
 void
Index: gcc/sel-sched-ir.c
===
--- gcc/sel-sched-ir.c	(revision 241233)
+++ gcc/sel-sched-ir.c	(working copy)
@@ -5762,6 +5762,10 @@ create_copy_of_insn_rtx (rtx insn_rtx)
   res = create_insn_rtx_from_pattern (copy_rtx (PATTERN (insn_rtx)),
   NULL_RTX);
 
+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */
+  rtx *ptail = _NOTES (res);
+  gcc_assert (*ptail == NULL_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.  */
@@ -5770,11 +5774,8 @@ create_copy_of_insn_rtx (rtx insn_rtx)
 	&& 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));
+	*ptail = duplicate_reg_note (link);
+	ptail =  (*ptail, 1);
   }
 
   return res;

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

[Eric Botcazou]

> Thanks for the feedback,  I'll try to work through this.

Thanks, but since there doesn't seem to be a consensus on the goal, feel free 
to disregard it and just implement what you need for your initial work.

-- 
Eric Botcazou

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

[Jiong Wang]

On 03/11/16 12:06, Eric Botcazou wrote:

What's your decision on this?

I think that we ought to standardize on a single order for note copying in the
RTL middle-end and the best way to enforce it is to have a single primitive in
rtlanal.c, with an optional filtering.  Bernd's patch is a step in the right
direction, but doesn't enforce the single order.  Maybe something based on a
macro calling duplicate_reg_note, but not clear whether it's really better.


Thanks for the feedback,  I'll try to work through this.

Regards,
Jiong

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

[Eric Botcazou]

>What's your decision on this?

I think that we ought to standardize on a single order for note copying in the 
RTL middle-end and the best way to enforce it is to have a single primitive in 
rtlanal.c, with an optional filtering.  Bernd's patch is a step in the right 
direction, but doesn't enforce the single order.  Maybe something based on a 
macro calling duplicate_reg_note, but not clear whether it's really better.

-- 
Eric Botcazou

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

[Jiong Wang]

On 21/10/16 13:30, Bernd Schmidt wrote:



On 10/21/2016 02:04 PM, Jiong Wang wrote:

+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */
+  rtx *ptail = _NOTES (new_rtx);
+  while (*ptail != NULL_RTX)
+ptail =  (*ptail, 1);


I was thinking along the lines of something like this (untested, 
emit-rtl.c part omitted). Eric can choose whether he likes either of 
these or wants something else.


Hi Eric,

  What's your decision on this?

  Thanks.

Regards,
Jiong




Bernd

Index: gcc/rtl.h
===
--- gcc/rtl.h(revision 241233)
+++ gcc/rtl.h(working copy)
@@ -3008,6 +3008,7 @@ extern rtx alloc_reg_note (enum reg_note
 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 rtx duplicate_reg_note (rtx_insn *, rtx);
 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);
Index: gcc/rtlanal.c
===
--- gcc/rtlanal.c(revision 241233)
+++ gcc/rtlanal.c(working copy)
@@ -2304,6 +2304,21 @@ add_shallow_copy_of_reg_note (rtx_insn *
 add_reg_note (insn, REG_NOTE_KIND (note), XEXP (note, 0));
 }

+/* Duplicate NOTE and return the copy.  */
+rtx
+duplicate_reg_note (rtx note)
+{
+  rtx n;
+  reg_note_kind kind = REG_NOTE_KIND (note);
+
+  if (GET_CODE (note) == INT_LIST)
+return gen_rtx_INT_LIST ((machine_mode) kind, XINT (note, 0), 
NULL_RTX);

+  else if (GET_CODE (note) == EXPR_LIST)
+return alloc_reg_note (kind, copy_insn_1 (XEXP (note, 0)), 
NULL_RTX);

+  else
+return alloc_reg_note (kind, XEXP (note, 0), NULL_RTX);
+}
+
 /* Remove register note NOTE from the REG_NOTES of INSN.  */

 void
Index: gcc/sel-sched-ir.c
===
--- gcc/sel-sched-ir.c(revision 241233)
+++ gcc/sel-sched-ir.c(working copy)
@@ -5762,6 +5762,11 @@ create_copy_of_insn_rtx (rtx insn_rtx)
   res = create_insn_rtx_from_pattern (copy_rtx (PATTERN (insn_rtx)),
   NULL_RTX);

+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */
+  rtx *ptail = _NOTES (new_rtx);
+  while (*ptail != NULL_RTX)
+ptail =  (*ptail, 1);
+
   /* 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. */
@@ -5770,11 +5775,8 @@ create_copy_of_insn_rtx (rtx insn_rtx)
 && 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));
+*ptail = duplicate_reg_note (link);
+ptail =  (*ptail, 1);
   }

   return res;

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

[Bernd Schmidt]

On 10/21/2016 02:04 PM, Jiong Wang wrote:

+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */
+  rtx *ptail = _NOTES (new_rtx);
+  while (*ptail != NULL_RTX)
+ptail =  (*ptail, 1);


I was thinking along the lines of something like this (untested, 
emit-rtl.c part omitted). Eric can choose whether he likes either of 
these or wants something else.



Bernd

Index: gcc/rtl.h
===
--- gcc/rtl.h   (revision 241233)
+++ gcc/rtl.h   (working copy)
@@ -3008,6 +3008,7 @@ extern rtx alloc_reg_note (enum reg_note
 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 rtx duplicate_reg_note (rtx_insn *, rtx);
 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);
Index: gcc/rtlanal.c
===
--- gcc/rtlanal.c   (revision 241233)
+++ gcc/rtlanal.c   (working copy)
@@ -2304,6 +2304,21 @@ add_shallow_copy_of_reg_note (rtx_insn *
 add_reg_note (insn, REG_NOTE_KIND (note), XEXP (note, 0));
 }

+/* Duplicate NOTE and return the copy.  */
+rtx
+duplicate_reg_note (rtx note)
+{
+  rtx n;
+  reg_note_kind kind = REG_NOTE_KIND (note);
+
+  if (GET_CODE (note) == INT_LIST)
+return gen_rtx_INT_LIST ((machine_mode) kind, XINT (note, 0), 
NULL_RTX);

+  else if (GET_CODE (note) == EXPR_LIST)
+return alloc_reg_note (kind, copy_insn_1 (XEXP (note, 0)), NULL_RTX);
+  else
+return alloc_reg_note (kind, XEXP (note, 0), NULL_RTX);
+}
+
 /* Remove register note NOTE from the REG_NOTES of INSN.  */

 void
Index: gcc/sel-sched-ir.c
===
--- gcc/sel-sched-ir.c  (revision 241233)
+++ gcc/sel-sched-ir.c  (working copy)
@@ -5762,6 +5762,11 @@ create_copy_of_insn_rtx (rtx insn_rtx)
   res = create_insn_rtx_from_pattern (copy_rtx (PATTERN (insn_rtx)),
   NULL_RTX);

+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */
+  rtx *ptail = _NOTES (new_rtx);
+  while (*ptail != NULL_RTX)
+ptail =  (*ptail, 1);
+
   /* 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.  */
@@ -5770,11 +5775,8 @@ create_copy_of_insn_rtx (rtx insn_rtx)
&& 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));
+   *ptail = duplicate_reg_note (link);
+   ptail =  (*ptail, 1);
   }

   return res;

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

[Jiong Wang]

On 21/10/16 11:13, Bernd Schmidt wrote:

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


Hi Bernd, Eric,

  How does the attached patch looks to you?  x86_64 bootstrap & regression OK.

  I borrowed Bernd' code to write the tail pointer directly.


2016-10-21  Bernd Schmidt  
Jiong Wang  
  
gcc/


PR middle-end/78016
* emit-rtl.c (emit_copy_of_insn_after): Copy REG_NOTES in order instead
of in reverse order.
* sel-sched-ir.c (create_copy_of_insn_rtx): Likewise.


diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 2d6d1eb6c1311871f15dbed13d7c084ed3845a86..4d849ca6e64273bedc5bf8b9a62a5cc5d4606129 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -6168,17 +6168,31 @@ emit_copy_of_insn_after (rtx_insn *insn, rtx_insn *after)
  which may be duplicated by the basic block reordering code.  */
   RTX_FRAME_RELATED_P (new_rtx) = RTX_FRAME_RELATED_P (insn);
 
+  /* Locate the end of existing REG_NOTES in NEW_RTX.  */
+  rtx *ptail = _NOTES (new_rtx);
+  while (*ptail != NULL_RTX)
+ptail =  (*ptail, 1);
+
   /* 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)));
+	rtx new_node;
+
+	if (GET_CODE (link) == INT_LIST)
+	  new_node = gen_rtx_INT_LIST ((machine_mode) REG_NOTE_KIND (link),
+   XINT (link, 0), NULL_RTX);
 	else
-	  add_shallow_copy_of_reg_note (new_rtx, link);
+	  new_node = alloc_reg_note (REG_NOTE_KIND (link),
+ (GET_CODE (link) == EXPR_LIST
+  ? copy_insn_1 (XEXP (link, 0))
+  : XEXP (link ,0)),
+ NULL_RTX);
+
+	*ptail = new_node;
+	ptail =  (new_node, 1);
   }
 
   INSN_CODE (new_rtx) = INSN_CODE (insn);
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 210b1e4edfb359a161cda4826704005ae9ab5a24..324ae8cf05209757a3a3f3dee97c9274876c7ed7 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -5761,6 +5761,11 @@ create_copy_of_insn_rtx (rtx insn_rtx)
   res = create_insn_rtx_from_pattern (copy_rtx (PATTERN (insn_rtx)),
   NULL_RTX);
 
+  /* Locate the end of existing REG_NOTES in RES.  */
+  rtx *ptail = _NOTES (res);
+  while (*ptail != NULL_RTX)
+ptail =  (*ptail, 1);
+
   /* 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.  */
@@ -5769,11 +5774,12 @@ create_copy_of_insn_rtx (rtx insn_rtx)
 	&& 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));
+	rtx new_node = alloc_reg_note (REG_NOTE_KIND (link),
+   (GET_CODE (link) == EXPR_LIST
+	? copy_insn_1 (XEXP (link, 0))
+	: XEXP (link ,0)), NULL_RTX);
+	*ptail = new_node;
+	ptail =  (new_node, 1);
   }
 
   return res;

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

[Bernd Schmidt]

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

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

[Jiong Wang]

On 21/10/16 08:43, Eric Botcazou wrote:

That's also overcomplicated.

Yes, I agree that's too heavy.


rtx *ptail = _NOTES (to_insn);
while (*ptail != NULL_RTX)
ptail =  (*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.

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

[Eric Botcazou]

> That's also overcomplicated.

Yes, I agree that's too heavy.

> rtx *ptail = _NOTES (to_insn);
> while (*ptail != NULL_RTX)
>ptail =  (*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

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

[Bernd Schmidt]

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 = _NOTES (to_insn);
while (*ptail != NULL_RTX)
  ptail =  (*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

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

[Jiong Wang]

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  

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.

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 *