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 = &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