This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Fix dbr_schedule mishandling of nonlocal gotos
- From: Richard Sandiford <rsandifo at redhat dot com>
- To: gcc-patches at gcc dot gnu dot org
- Date: Wed, 26 May 2004 22:19:08 +0100
- Subject: 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.