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

Bernd Schmidt bschmidt@redhat.com
Thu Oct 20 15:42:00 GMT 2016


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



More information about the Gcc-patches mailing list