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

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  <bschmidt@redhat.com>
             Jiong Wang  <jiong.wang@arm.com>
   
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.

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 = &REG_NOTES (new_rtx);

>> +  while (*ptail != NULL_RTX)

>> +    ptail = &XEXP (*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 = &REG_NOTES (new_rtx);

> +  while (*ptail != NULL_RTX)

> +    ptail = &XEXP (*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 = &XEXP (*ptail, 1);

>        }

>

>    return res;

>    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

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

> 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