Patchwork simplify emit_delay_sequence

login
register
mail settings
Submitter Steven Bosscher
Date April 16, 2013, 10:15 p.m.
Message ID <CABu31nNZfrCX8Y9ZhT39WDq7EMUa=AuGq9HOpgAFv1nUaUeGMw@mail.gmail.com>
Download mbox | patch
Permalink /patch/237117/
State New
Headers show

Comments

Steven Bosscher - April 16, 2013, 10:15 p.m.
Hello,

With the recent add_insn_* cleanups, we can now simplify
reorg.c:emit_delay_sequence.

Without this patch, emit_delay_sequence hacks the insns chain
"manually", setting PREV_INSN and NEXT_INSN to chain everything
together properly. But emit-rtl provides an abstraction of sorts for
that, and emit_delay_sequence should use it. This wasn't possible
before, because inserting a SEQUENCE wasn't allowed. But with a 5-line
change in emit-rtl.c, inserting a SEQUENCE via add_insn,
add_insn_before, and add_insn_after now works just fine.:

This patch is also necessary for my new delay-slot scheduler to keep
basic block boundaries correctly up-to-date. The emit-rtl API does
that already.

Cross-tested powerpc64 x mips. Currently running bootstrap&test on
sparc64-unknown-linux-gnu. OK if it passes?

Ciao!
Steven
* emit-rtl.c (link_insn_into_chain): Handle chaining of SEQUENCEs.
	* reorg.c (emit_delay_sequence): Simplify with emit-rtl API.
Eric Botcazou - April 17, 2013, 8:39 a.m.
> This patch is also necessary for my new delay-slot scheduler to keep
> basic block boundaries correctly up-to-date. The emit-rtl API does
> that already.
> 
> Cross-tested powerpc64 x mips. Currently running bootstrap&test on
> sparc64-unknown-linux-gnu. OK if it passes?

Yes, modulo

@@ -538,6 +502,8 @@ emit_delay_sequence (rtx insn, rtx list, int lengt
 	INSN_LOCATION (seq_insn) = INSN_LOCATION (tem);
       INSN_LOCATION (tem) = 0;
 
+      /* Remove any REG_DEAD notes because we can't rely on them now
+	 that the insn has been moved.  */
       for (note = REG_NOTES (tem); note; note = next)
 	{
 	  next = XEXP (note, 1);

Did you mean to move the comment instead of duplicating it?
Steven Bosscher - April 17, 2013, 7:34 p.m.
On Wed, Apr 17, 2013 at 10:39 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> @@ -538,6 +502,8 @@ emit_delay_sequence (rtx insn, rtx list, int lengt
>         INSN_LOCATION (seq_insn) = INSN_LOCATION (tem);
>        INSN_LOCATION (tem) = 0;
>
> +      /* Remove any REG_DEAD notes because we can't rely on them now
> +        that the insn has been moved.  */
>        for (note = REG_NOTES (tem); note; note = next)
>         {
>           next = XEXP (note, 1);
>
> Did you mean to move the comment instead of duplicating it?

No, it's a merge mistake. My copy of this function in sched-dbr
doesn't have the switch (it doesn't delete insns and re-inserts them,
it simply moves them, so updating label notes is not necessary) and I
just copied the comment before the loop. Then I merged back some of
the changes to reorg.c and messed up :-)

Thanks for the review. Committed.

Ciao!
Steven

Patch

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 198008)
+++ emit-rtl.c	(working copy)
@@ -3806,6 +3806,13 @@  link_insn_into_chain (rtx insn, rtx prev, rtx next
       if (NONJUMP_INSN_P (next) && GET_CODE (PATTERN (next)) == SEQUENCE)
 	PREV_INSN (XVECEXP (PATTERN (next), 0, 0)) = insn;
     }
+
+  if (NONJUMP_INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE)
+    {
+      rtx sequence = PATTERN (insn);
+      PREV_INSN (XVECEXP (sequence, 0, 0)) = prev;
+      NEXT_INSN (XVECEXP (sequence, 0, XVECLEN (sequence, 0) - 1)) = next;
+    }
 }
 
 /* Add INSN to the end of the doubly-linked list.
Index: reorg.c
===================================================================
--- reorg.c	(revision 198008)
+++ reorg.c	(working copy)
@@ -458,69 +458,32 @@  find_end_label (rtx kind)
 /* Put INSN and LIST together in a SEQUENCE rtx of LENGTH, and replace
    the pattern of INSN with the SEQUENCE.
 
-   Chain the insns so that NEXT_INSN of each insn in the sequence points to
-   the next and NEXT_INSN of the last insn in the sequence points to
-   the first insn after the sequence.  Similarly for PREV_INSN.  This makes
-   it easier to scan all insns.
-
    Returns the SEQUENCE that replaces INSN.  */
 
 static rtx
 emit_delay_sequence (rtx insn, rtx list, int length)
 {
-  int i = 1;
-  rtx li;
-  int had_barrier = 0;
-
   /* Allocate the rtvec to hold the insns and the SEQUENCE.  */
   rtvec seqv = rtvec_alloc (length + 1);
   rtx seq = gen_rtx_SEQUENCE (VOIDmode, seqv);
   rtx seq_insn = make_insn_raw (seq);
-  rtx first = get_insns ();
-  rtx last = get_last_insn ();
 
-  /* Make a copy of the insn having delay slots.  */
-  rtx delay_insn = copy_rtx (insn);
+  /* If DELAY_INSN has a location, use it for SEQ_INSN.  If DELAY_INSN does
+     not have a location, but one of the delayed insns does, we pick up a
+     location from there later.  */
+  INSN_LOCATION (seq_insn) = INSN_LOCATION (insn);
 
-  /* If INSN is followed by a BARRIER, delete the BARRIER since it will only
-     confuse further processing.  Update LAST in case it was the last insn.
-     We will put the BARRIER back in later.  */
-  if (NEXT_INSN (insn) && BARRIER_P (NEXT_INSN (insn)))
-    {
-      delete_related_insns (NEXT_INSN (insn));
-      last = get_last_insn ();
-      had_barrier = 1;
-    }
+  /* Unlink INSN from the insn chain, so that we can put it into
+     the SEQUENCE.   Remember where we want to emit SEQUENCE in AFTER.  */
+  rtx after = PREV_INSN (insn);
+  remove_insn (insn);
+  NEXT_INSN (insn) = PREV_INSN (insn) = NULL;
 
-  /* Splice our SEQUENCE into the insn stream where INSN used to be.  */
-  NEXT_INSN (seq_insn) = NEXT_INSN (insn);
-  PREV_INSN (seq_insn) = PREV_INSN (insn);
-
-  if (insn != last)
-    PREV_INSN (NEXT_INSN (seq_insn)) = seq_insn;
-
-  if (insn != first)
-    NEXT_INSN (PREV_INSN (seq_insn)) = seq_insn;
-
-  /* Note the calls to set_new_first_and_last_insn must occur after
-     SEQ_INSN has been completely spliced into the insn stream.
-
-     Otherwise CUR_INSN_UID will get set to an incorrect value because
-     set_new_first_and_last_insn will not find SEQ_INSN in the chain.  */
-  if (insn == last)
-    set_new_first_and_last_insn (first, seq_insn);
-
-  if (insn == first)
-    set_new_first_and_last_insn (seq_insn, last);
-
   /* Build our SEQUENCE and rebuild the insn chain.  */
-  XVECEXP (seq, 0, 0) = delay_insn;
-  INSN_DELETED_P (delay_insn) = 0;
-  PREV_INSN (delay_insn) = PREV_INSN (seq_insn);
-
-  INSN_LOCATION (seq_insn) = INSN_LOCATION (delay_insn);
-
-  for (li = list; li; li = XEXP (li, 1), i++)
+  int i = 1;
+  start_sequence ();
+  XVECEXP (seq, 0, 0) = emit_insn (insn);
+  for (rtx li = list; li; li = XEXP (li, 1), i++)
     {
       rtx tem = XEXP (li, 0);
       rtx note, next;
@@ -528,9 +491,10 @@  emit_delay_sequence (rtx insn, rtx list, int lengt
       /* Show that this copy of the insn isn't deleted.  */
       INSN_DELETED_P (tem) = 0;
 
-      XVECEXP (seq, 0, i) = tem;
-      PREV_INSN (tem) = XVECEXP (seq, 0, i - 1);
-      NEXT_INSN (XVECEXP (seq, 0, i - 1)) = tem;
+      /* Unlink insn from its original place, and re-emit it into
+	 the sequence.  */
+      NEXT_INSN (tem) = PREV_INSN (tem) = NULL;
+      XVECEXP (seq, 0, i) = emit_insn (tem);
 
       /* SPARC assembler, for instance, emit warning when debug info is output
          into the delay slot.  */
@@ -538,6 +502,8 @@  emit_delay_sequence (rtx insn, rtx list, int lengt
 	INSN_LOCATION (seq_insn) = INSN_LOCATION (tem);
       INSN_LOCATION (tem) = 0;
 
+      /* Remove any REG_DEAD notes because we can't rely on them now
+	 that the insn has been moved.  */
       for (note = REG_NOTES (tem); note; note = next)
 	{
 	  next = XEXP (note, 1);
@@ -561,29 +527,12 @@  emit_delay_sequence (rtx insn, rtx list, int lengt
 	    }
 	}
     }
-
-  NEXT_INSN (XVECEXP (seq, 0, length)) = NEXT_INSN (seq_insn);
-
-  /* If the previous insn is a SEQUENCE, update the NEXT_INSN pointer on the
-     last insn in that SEQUENCE to point to us.  Similarly for the first
-     insn in the following insn if it is a SEQUENCE.  */
-
-  if (PREV_INSN (seq_insn) && NONJUMP_INSN_P (PREV_INSN (seq_insn))
-      && GET_CODE (PATTERN (PREV_INSN (seq_insn))) == SEQUENCE)
-    NEXT_INSN (XVECEXP (PATTERN (PREV_INSN (seq_insn)), 0,
-			XVECLEN (PATTERN (PREV_INSN (seq_insn)), 0) - 1))
-      = seq_insn;
-
-  if (NEXT_INSN (seq_insn) && NONJUMP_INSN_P (NEXT_INSN (seq_insn))
-      && GET_CODE (PATTERN (NEXT_INSN (seq_insn))) == SEQUENCE)
-    PREV_INSN (XVECEXP (PATTERN (NEXT_INSN (seq_insn)), 0, 0)) = seq_insn;
-
-  /* If there used to be a BARRIER, put it back.  */
-  if (had_barrier)
-    emit_barrier_after (seq_insn);
-
+  end_sequence ();
   gcc_assert (i == length + 1);
 
+  /* Splice our SEQUENCE into the insn stream where INSN used to be.  */
+  add_insn_after (seq_insn, after, NULL);
+
   return seq_insn;
 }