diff mbox

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

Message ID b1945366-00df-0cd4-3b64-361ca19b0413@redhat.com
State New
Headers show

Commit Message

Bernd Schmidt Nov. 3, 2016, 1:01 p.m. UTC 
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

Comments

Eric Botcazou Nov. 3, 2016, 2 p.m. UTC | #1 
> 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
Jiong Wang Nov. 3, 2016, 5:44 p.m. UTC | #2 
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 = &REG_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 = &XEXP (*ptail, 1);

Regards,
Jiong
Bernd Schmidt Nov. 7, 2016, 5:04 p.m. UTC | #3 
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
Jiong Wang Nov. 7, 2016, 5:23 p.m. UTC | #4 
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
diff mbox

Patch

	* 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 = &REG_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 = &XEXP (*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 = &REG_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 = &XEXP (*ptail, 1);
       }
 
   return res;