This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Fix dbr_schedule mishandling of nonlocal gotos


Several nonlocal-goto tests are failing for MIPS because the
gotos are being redirected to the end of the current function.
The guilty code is in relax_delay_slots:

	  /* If this jump goes to another unconditional jump, thread it, but
	     don't convert a jump into a RETURN here.  */
	  trial = follow_jumps (target_label);
	  /* We use next_real_insn instead of next_active_insn, so that
	     the special USE insns emitted by reorg won't be ignored.
	     If they are ignored, then they will get deleted if target_label
	     is now unreachable, and that would cause mark_target_live_regs
	     to fail.  */
	  trial = prev_label (next_real_insn (trial));
--->	  if (trial == 0 && target_label != 0)
--->	    trial = find_end_label ();

"trial" can be null for three reasons:

  (1) Because follow_jumps() returned null.  It wasn't obvious from
      reading the comment above follow_jumps that this was indeed
      a possiblity, but from reading the implementation, it returns
      null for labels that eventually thread to a return insn.
      (The patch below adds a comment to that effect.)

  (2) Because follow_jumps() returns a label at the end of the
      function, causing prev_label (next_real_insn (...)) to
      return null.

  (3) Because TARGET_LABEL is a nonlocal label, which also causes
      prev_label (next_real_insn (...)) to be null.

Clearly the code is only expecting (1) and (2); it doesn't do the right
thing for (3).  Also, find_end_label() seems a bit heavyweight for (2),
given that we already had the label to hand.

The attached patch therefore adds a new function, skip_consecutive_labels,
to emit-rtl.c and uses that instead of prev_label (next_real_insn (...)).
The null check is then only needed for (1).

There were three other uses of prev_label (next_real_insn (...))
besides the one quoted above:

  - One very similar hunk elsewhere in relax_delay_slots().  It needs
    the same fix.

  - One in dbr_schedule().  It won't mishandle nonlocal gotos, but I think
    using the new function anyway will make things more readable.

  - One in the following hunk, again from relax_delay_slots():

      /* See if this is an unconditional jump around a single insn which is
         identical to the one in its delay slot.  In this case, we can just
         delete the branch and the insn in its delay slot.  */
      if (next && GET_CODE (next) == INSN
          && prev_label (next_active_insn (next)) == target_label
          && simplejump_p (insn)
          && XVECLEN (pat, 0) == 2
          && rtx_equal_p (PATTERN (next), PATTERN (XVECEXP (pat, 0, 1))))
        {
          delete_related_insns (insn);
          continue;
        }

    In this case the construct is looking for a label after an INSN,
    so it should stay the way it is.

Tested on mips-sgi-irix6.5 and mipsisa64-elf.  OK to install?

Richard


	* rtl.h (skip_consecutive_labels): Declare.
	* emit-rtl.c (skip_consecutive_labels): New function.
	* reorg.c (relax_delay_slots, dbr_schedule): Use it.
	* jump.c (follow_jumps): Say what null return values mean.

Index: rtl.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/rtl.h,v
retrieving revision 1.474
diff -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.474 rtl.h
--- rtl.h	19 May 2004 17:53:45 -0000	1.474
+++ rtl.h	25 May 2004 19:24:52 -0000
@@ -1695,6 +1695,7 @@ extern rtx next_active_insn (rtx);
 extern int active_insn_p (rtx);
 extern rtx prev_label (rtx);
 extern rtx next_label (rtx);
+extern rtx skip_consecutive_labels (rtx);
 extern rtx next_cc0_user (rtx);
 extern rtx prev_cc0_setter (rtx);
 
Index: emit-rtl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/emit-rtl.c,v
retrieving revision 1.390
diff -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.390 emit-rtl.c
--- emit-rtl.c	13 May 2004 06:39:38 -0000	1.390
+++ emit-rtl.c	25 May 2004 19:24:53 -0000
@@ -3084,6 +3084,21 @@ prev_label (rtx insn)
 
   return insn;
 }
+
+/* Return the last label to mark the same position as LABEL.  Return null
+   if LABEL itself is null.  */
+
+rtx
+skip_consecutive_labels (rtx label)
+{
+  rtx insn;
+
+  for (insn = label; insn != 0 && !INSN_P (insn); insn = NEXT_INSN (insn))
+    if (LABEL_P (insn))
+      label = insn;
+
+  return label;
+}
 
 #ifdef HAVE_cc0
 /* INSN uses CC0 and is being moved into a delay slot.  Set up REG_CC_SETTER
Index: reorg.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reorg.c,v
retrieving revision 1.95
diff -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.95 reorg.c
--- reorg.c	20 May 2004 18:36:50 -0000	1.95
+++ reorg.c	25 May 2004 19:24:54 -0000
@@ -3079,11 +3079,7 @@ relax_delay_slots (rtx first)
 	  && (condjump_p (insn) || condjump_in_parallel_p (insn))
 	  && (target_label = JUMP_LABEL (insn)) != 0)
 	{
-	  target_label = follow_jumps (target_label);
-	  /* See comment further down why we must use next_real_insn here,
-	     instead of next_active_insn.  */
-	  target_label = prev_label (next_real_insn (target_label));
-
+	  target_label = skip_consecutive_labels (follow_jumps (target_label));
 	  if (target_label == 0)
 	    target_label = find_end_label ();
 
@@ -3231,14 +3227,8 @@ relax_delay_slots (rtx first)
 	{
 	  /* If this jump goes to another unconditional jump, thread it, but
 	     don't convert a jump into a RETURN here.  */
-	  trial = follow_jumps (target_label);
-	  /* We use next_real_insn instead of next_active_insn, so that
-	     the special USE insns emitted by reorg won't be ignored.
-	     If they are ignored, then they will get deleted if target_label
-	     is now unreachable, and that would cause mark_target_live_regs
-	     to fail.  */
-	  trial = prev_label (next_real_insn (trial));
-	  if (trial == 0 && target_label != 0)
+	  trial = skip_consecutive_labels (follow_jumps (target_label));
+	  if (trial == 0)
 	    trial = find_end_label ();
 
 	  if (trial != target_label
@@ -3621,7 +3611,7 @@ dbr_schedule (rtx first, FILE *file)
       if (GET_CODE (insn) == JUMP_INSN
 	  && (condjump_p (insn) || condjump_in_parallel_p (insn))
 	  && JUMP_LABEL (insn) != 0
-	  && ((target = prev_label (next_active_insn (JUMP_LABEL (insn))))
+	  && ((target = skip_consecutive_labels (JUMP_LABEL (insn)))
 	      != JUMP_LABEL (insn)))
 	redirect_jump (insn, target, 1);
     }
Index: jump.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/jump.c,v
retrieving revision 1.244
diff -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.244 jump.c
--- jump.c	13 May 2004 06:39:43 -0000	1.244
+++ jump.c	25 May 2004 19:24:55 -0000
@@ -999,6 +999,7 @@ sets_cc0_p (rtx x)
 
 /* Follow any unconditional jump at LABEL;
    return the ultimate label reached by any such chain of jumps.
+   Return null if the chain ultimately leads to a return instruction.
    If LABEL is not followed by a jump, return LABEL.
    If the chain loops or we can't find end, return LABEL,
    since that tells caller to avoid changing the insn.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]