diff mbox

Make rtl_split_edge work for jumps that fall through (PR72749)

Message ID c6964da40e39ebe7706e8e8073ab4c2cdce64aba.1484415306.git.segher@kernel.crashing.org
State New
Headers show

Commit Message

Segher Boessenkool Jan. 14, 2017, 5:55 p.m. UTC 
If a jump always falls through but that cannot be optimised away, like
is the case with the PowerPC bdnz insn if its jump target is the same as
the fallthrough, sched gets confused if it schedules some instructions
from before that jump instruction to behind it: it splits the
fallthrough branch, but the jump target isn't updated, and things fall
apart as in the PR.  This patch fixes it.

The second patch fragment makes -fsched-verbose=N work for N>=4; the
currently scheduled fragment can now contain a label.  Everything else
seems to work fine with that.

Tested on powerpc*-linux (with the testcase Alan checked in earlier
today); also bootstrapped on powerpc64-linux.  Is this okay for trunk?


Segher


2017-01-14  Segher Boessenkool  <segher@kernel.crashing.org>

	PR target/72749
	* cfgrtl.c (rtl_split_edge): Also patch jump insns that jump to the
	fallthrough.
	* haifa-sched.c (dump_insn_stream): Don't crash if there is a label
	in the currently scheduled RTL fragment.

---
 gcc/cfgrtl.c      | 2 +-
 gcc/haifa-sched.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Segher Boessenkool Jan. 14, 2017, 7:09 p.m. UTC | #1 
On Sat, Jan 14, 2017 at 05:55:48PM +0000, Segher Boessenkool wrote:
> -	      && extract_asm_operands (PATTERN (last)) != NULL_RTX
> +	      && (extract_asm_operands (last) || JUMP_LABEL (last) == before)

Somehow I removed the PATTERN there; retesting with it.  I'll test it
on x86 as well, maybe it triggers there (it doesn't on PowerPC obviously).


Segher
Segher Boessenkool Jan. 14, 2017, 10:29 p.m. UTC | #2 
On Sat, Jan 14, 2017 at 01:09:14PM -0600, Segher Boessenkool wrote:
> On Sat, Jan 14, 2017 at 05:55:48PM +0000, Segher Boessenkool wrote:
> > -	      && extract_asm_operands (PATTERN (last)) != NULL_RTX
> > +	      && (extract_asm_operands (last) || JUMP_LABEL (last) == before)
> 
> Somehow I removed the PATTERN there; retesting with it.  I'll test it
> on x86 as well, maybe it triggers there (it doesn't on PowerPC obviously).

Tested that on powerpc64-linux and x86_64-linux now; no failures.


Segher
Jeff Law Jan. 15, 2017, 4:47 a.m. UTC | #3 
On 01/14/2017 10:55 AM, Segher Boessenkool wrote:
> If a jump always falls through but that cannot be optimised away, like
> is the case with the PowerPC bdnz insn if its jump target is the same as
> the fallthrough, sched gets confused if it schedules some instructions
> from before that jump instruction to behind it: it splits the
> fallthrough branch, but the jump target isn't updated, and things fall
> apart as in the PR.  This patch fixes it.
>
> The second patch fragment makes -fsched-verbose=N work for N>=4; the
> currently scheduled fragment can now contain a label.  Everything else
> seems to work fine with that.
>
> Tested on powerpc*-linux (with the testcase Alan checked in earlier
> today); also bootstrapped on powerpc64-linux.  Is this okay for trunk?
>
>
> Segher
>
>
> 2017-01-14  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	PR target/72749
> 	* cfgrtl.c (rtl_split_edge): Also patch jump insns that jump to the
> 	fallthrough.
> 	* haifa-sched.c (dump_insn_stream): Don't crash if there is a label
> 	in the currently scheduled RTL fragment.
So presumably the semantics in this case can only mean one thing -- both 
the fallthru and the jump have to go to the same place after splitting. 
Right?  ISTM that ought to be documented in rtl_split_edge somewhere.

In this case is the edge a fallthru or branch edge?

And, no we don't make any guarantee about degenerate jumps -- a jump 
with a side effect in particular can easily survive.  I wouldn't be 
surprised if there's various bugs lurking for cases where we have a 
degenerate jump like that.  It's just not common enough to have been 
heavily exercised.

Patch with PATTERN fix is OK.

jeff
Segher Boessenkool Jan. 15, 2017, 5:27 a.m. UTC | #4 
On Sat, Jan 14, 2017 at 09:47:41PM -0700, Jeff Law wrote:
> >	PR target/72749
> >	* cfgrtl.c (rtl_split_edge): Also patch jump insns that jump to the
> >	fallthrough.
> >	* haifa-sched.c (dump_insn_stream): Don't crash if there is a label
> >	in the currently scheduled RTL fragment.
> So presumably the semantics in this case can only mean one thing -- both 
> the fallthru and the jump have to go to the same place after splitting. 
> Right?  ISTM that ought to be documented in rtl_split_edge somewhere.

We could call patch_jump_insn in *all* cases, except for simple jumps
it does

	  gcc_assert (JUMP_LABEL (insn) == old_label);

which is a useful check (for the other callers), and patch_jump_insn
isn't a very cheap routine to call anyway, better not do it too often.

> In this case is the edge a fallthru or branch edge?

Both!  You get only one edge if both jump targets are the same.

> And, no we don't make any guarantee about degenerate jumps -- a jump 
> with a side effect in particular can easily survive.  I wouldn't be 
> surprised if there's various bugs lurking for cases where we have a 
> degenerate jump like that.  It's just not common enough to have been 
> heavily exercised.
> 
> Patch with PATTERN fix is OK.

Thanks, I'll commit it tomorrow.


Segher
Jeff Law Jan. 15, 2017, 5:52 a.m. UTC | #5 
On 01/14/2017 10:27 PM, Segher Boessenkool wrote:
> On Sat, Jan 14, 2017 at 09:47:41PM -0700, Jeff Law wrote:
>>> 	PR target/72749
>>> 	* cfgrtl.c (rtl_split_edge): Also patch jump insns that jump to the
>>> 	fallthrough.
>>> 	* haifa-sched.c (dump_insn_stream): Don't crash if there is a label
>>> 	in the currently scheduled RTL fragment.
>> So presumably the semantics in this case can only mean one thing -- both
>> the fallthru and the jump have to go to the same place after splitting.
>> Right?  ISTM that ought to be documented in rtl_split_edge somewhere.
>
> We could call patch_jump_insn in *all* cases, except for simple jumps
> it does
>
> 	  gcc_assert (JUMP_LABEL (insn) == old_label);
>
> which is a useful check (for the other callers), and patch_jump_insn
> isn't a very cheap routine to call anyway, better not do it too often.
>
>> In this case is the edge a fallthru or branch edge?
>
> Both!  You get only one edge if both jump targets are the same.
Right.  To be more precise, my question was does it  have EDGE_FALLTHRU 
set?

Jeff
Segher Boessenkool Jan. 15, 2017, 6:45 a.m. UTC | #6 
On Sat, Jan 14, 2017 at 10:52:04PM -0700, Jeff Law wrote:
> >>In this case is the edge a fallthru or branch edge?
> >
> >Both!  You get only one edge if both jump targets are the same.
> Right.  To be more precise, my question was does it  have EDGE_FALLTHRU 
> set?

Yes:

 succ:       6 [92.5%]  (FALLTHRU,DFS_BACK,CAN_FALLTHRU)

Just after doloop created it it was two edges.  After fwprop it is just
one.  outof_cfglayout makes it two again (to different targets).  bbro
makes it one yet again.


Segher
diff mbox

Patch

diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index 7604346..dbbba59 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -1931,7 +1931,7 @@  rtl_split_edge (edge edge_in)
 	  if (last
 	      && JUMP_P (last)
 	      && edge_in->dest != EXIT_BLOCK_PTR_FOR_FN (cfun)
-	      && extract_asm_operands (PATTERN (last)) != NULL_RTX
+	      && (extract_asm_operands (last) || JUMP_LABEL (last) == before)
 	      && patch_jump_insn (last, before, bb))
 	    df_set_bb_dirty (edge_in->src);
 	}
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 1b13e32..07de1bb 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -6495,7 +6495,7 @@  dump_insn_stream (rtx_insn *head, rtx_insn *tail)
 
       if (sched_verbose >= 4)
 	{
-	  if (NOTE_P (insn) || recog_memoized (insn) < 0)
+	  if (NOTE_P (insn) || LABEL_P (insn) || recog_memoized (insn) < 0)
 	    fprintf (sched_dump, "nothing");
 	  else
 	    print_reservation (sched_dump, insn);