diff mbox

Fix cfg_layout_merge_blocks (PR rtl-optimization/52139)

Message ID alpine.LNX.2.00.1202081021330.4999@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Feb. 8, 2012, 9:27 a.m. UTC 
On Tue, 7 Feb 2012, Jakub Jelinek wrote:

> Hi!
> 
> On the following testcase we hit two bugs during cfglayout merge_blocks.
> 
> One is that b->il.rtl->header has some jumptable in it, followed by
> barrier.  We call emit_insn_after_noloc to insert the whole b's header
> after BB_END (a) and then delete_insn_chain it, with the intention that only
> non-deletable insns like deleted label notes are kept around.  Unfortunately
> delete_insn/remove_insn it uses isn't prepared to handle BARRIERs as part of
> a bb (i.e. if BB_END is equal to some barrier because of the
> emit_insn_after_noloc call, delete_insn_chain won't update BB_END properly).
> As barriers aren't part of a BB, instead of adjusting remove_insn this patch
> adjusts BB_END not to point to a barrier before calling delete_insn_chain.
> 
> The second bug is that remove_insn ICEs if deleting an insn with NEXT_INSN
> NULL, unless that insn is part of a current sequence (or some sequence in
> the sequence stack).  In the first version of the patch I've tried to
> avoid calling delete_insn on insns that have NEXT_INSN NULL, but given that
> having NULL NEXT_INSN is a pretty common situation when in cfglayout mode
> if the insn is at BB_END, I think it is better to allow that in remove_insn.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2012-02-07  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/52139
> 	* emit-rtl.c (remove_insn): Allow removing insns
> 	with NEXT_INSN NULL, if they are at BB_END.
> 	* cfgrtl.c (cfg_layout_merge_blocks): If BB_END
> 	is a BARRIER after emit_insn_after_noloc, move BB_END
> 	to the last non-BARRIER insn before it.  Cleanup.
> 
> 	* gcc.dg/pr52139.c: New test.
> 
> --- gcc/emit-rtl.c.jj	2012-02-07 16:05:47.913534092 +0100
> +++ gcc/emit-rtl.c	2012-02-07 16:14:32.529734964 +0100
> @@ -1,7 +1,7 @@
>  /* Emit RTL for the GCC expander.
>     Copyright (C) 1987, 1988, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
>     1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009,
> -   2010, 2011
> +   2010, 2011, 2012
>     Free Software Foundation, Inc.
>  
>  This file is part of GCC.
> @@ -3957,7 +3957,19 @@ remove_insn (rtx insn)
>  	    break;
>  	  }
>  
> -      gcc_assert (stack);
> +      if (stack == NULL)
> +	{
> +	  /* In cfglayout mode allow remove_insn of
> +	     insns at the end of bb.  */
> +	  if (BARRIER_P (insn))
> +	    {
> +	      gcc_assert (prev);
> +	      bb = BLOCK_FOR_INSN (prev);
> +	    }
> +	  else
> +	    bb = BLOCK_FOR_INSN (insn);
> +	  gcc_assert (bb && BB_END (bb) == insn);
> +	}

Isn't it cheaper to do that before we scan all the sequences?  Thus,
I'd expect BLOCK_FOR_INSN to be NULL if the insn is in a sequence?
Like simply


?  I realize that doesn't do the extra assertions, but this is a
pretty core routine and checking of invariants would more belong
to RTL checking.

Richard.


>      }
>    if (!BARRIER_P (insn)
>        && (bb = BLOCK_FOR_INSN (insn)))
> --- gcc/cfgrtl.c.jj	2012-02-07 16:05:47.977533716 +0100
> +++ gcc/cfgrtl.c	2012-02-07 17:03:52.925956927 +0100
> @@ -2818,6 +2818,7 @@ static void
>  cfg_layout_merge_blocks (basic_block a, basic_block b)
>  {
>    bool forwarder_p = (b->flags & BB_FORWARDER_BLOCK) != 0;
> +  rtx insn;
>  
>    gcc_checking_assert (cfg_layout_can_merge_blocks_p (a, b));
>  
> @@ -2871,6 +2872,11 @@ cfg_layout_merge_blocks (basic_block a,
>        rtx first = BB_END (a), last;
>  
>        last = emit_insn_after_noloc (b->il.rtl->header, BB_END (a), a);
> +      /* The above might add a BARRIER as BB_END, but as barriers
> +	 aren't valid parts of a bb, remove_insn doesn't update
> +	 BB_END if it is a barrier.  So adjust BB_END here.  */
> +      while (BB_END (a) != first && BARRIER_P (BB_END (a)))
> +	BB_END (a) = PREV_INSN (BB_END (a));
>        delete_insn_chain (NEXT_INSN (first), last, false);
>        b->il.rtl->header = NULL;
>      }
> @@ -2878,40 +2884,28 @@ cfg_layout_merge_blocks (basic_block a,
>    /* In the case basic blocks are not adjacent, move them around.  */
>    if (NEXT_INSN (BB_END (a)) != BB_HEAD (b))
>      {
> -      rtx first = unlink_insn_chain (BB_HEAD (b), BB_END (b));
> +      insn = unlink_insn_chain (BB_HEAD (b), BB_END (b));
>  
> -      emit_insn_after_noloc (first, BB_END (a), a);
> -      /* Skip possible DELETED_LABEL insn.  */
> -      if (!NOTE_INSN_BASIC_BLOCK_P (first))
> -	first = NEXT_INSN (first);
> -      gcc_assert (NOTE_INSN_BASIC_BLOCK_P (first));
> -      BB_HEAD (b) = NULL;
> -
> -      /* emit_insn_after_noloc doesn't call df_insn_change_bb.
> -         We need to explicitly call. */
> -      update_bb_for_insn_chain (NEXT_INSN (first),
> -				BB_END (b),
> -				a);
> -
> -      delete_insn (first);
> +      emit_insn_after_noloc (insn, BB_END (a), a);
>      }
>    /* Otherwise just re-associate the instructions.  */
>    else
>      {
> -      rtx insn;
> -
> -      update_bb_for_insn_chain (BB_HEAD (b), BB_END (b), a);
> -
>        insn = BB_HEAD (b);
> -      /* Skip possible DELETED_LABEL insn.  */
> -      if (!NOTE_INSN_BASIC_BLOCK_P (insn))
> -	insn = NEXT_INSN (insn);
> -      gcc_assert (NOTE_INSN_BASIC_BLOCK_P (insn));
> -      BB_HEAD (b) = NULL;
>        BB_END (a) = BB_END (b);
> -      delete_insn (insn);
>      }
>  
> +  /* emit_insn_after_noloc doesn't call df_insn_change_bb.
> +     We need to explicitly call. */
> +  update_bb_for_insn_chain (insn, BB_END (b), a);
> +
> +  /* Skip possible DELETED_LABEL insn.  */
> +  if (!NOTE_INSN_BASIC_BLOCK_P (insn))
> +    insn = NEXT_INSN (insn);
> +  gcc_assert (NOTE_INSN_BASIC_BLOCK_P (insn));
> +  BB_HEAD (b) = NULL;
> +  delete_insn (insn);
> +
>    df_bb_delete (b->index);
>  
>    /* Possible tablejumps and barriers should appear after the block.  */
> --- gcc/testsuite/gcc.dg/pr52139.c.jj	2012-02-07 16:14:32.537734917 +0100
> +++ gcc/testsuite/gcc.dg/pr52139.c	2012-02-07 16:14:32.537734917 +0100
> @@ -0,0 +1,49 @@
> +/* PR rtl-optimization/52139 */
> +/* { dg-do compile } */
> +/* { dg-options "-O -fno-tree-dominator-opts -fno-tree-fre" } */
> +/* { dg-additional-options "-fpic" { target fpic } } */
> +
> +void *p;
> +
> +void
> +foo (int a)
> +{
> +  switch (a)
> +    {
> +    case 0:
> +    a0:
> +    case 1:
> +    a1:
> +      p = &&a1;
> +    case 2:
> +    a2:
> +      p = &&a2;
> +    case 3:
> +    a3:
> +      p = &&a3;
> +    case 4:
> +    a4:
> +      p = &&a4;
> +    case 5:
> +    a5:
> +      p = &&a5;
> +    case 6:
> +    a6:
> +      p = &&a6;
> +    case 7:
> +    a7:
> +      p = &&a7;
> +    case 8:
> +    a8:
> +      p = &&a8;
> +    case 9:
> +    a9:
> +      p = &&a9;
> +    case 10:
> +    a10:
> +      p = &&a10;
> +    default:
> +      p = &&a0;
> +    }
> +  goto *p;
> +}
> 
> 	Jakub
> 
>

Comments

Jakub Jelinek Feb. 8, 2012, 10:18 a.m. UTC | #1 
On Wed, Feb 08, 2012 at 10:27:42AM +0100, Richard Guenther wrote:
> Isn't it cheaper to do that before we scan all the sequences?  Thus,
> I'd expect BLOCK_FOR_INSN to be NULL if the insn is in a sequence?
> Like simply

Actually, on a third look, the emit-rtl.c changes aren't needed,
apparently NEXT_INSN even in cfglayout mode is NULL for the in bb
insns only if it is equal to get_last_insn (), unless in the header/footer
sequences, but for those one shouldn't call remove_insn.

To fix this bug the
+      /* The above might add a BARRIER as BB_END, but as barriers
+        aren't valid parts of a bb, remove_insn doesn't update
+        BB_END if it is a barrier.  So adjust BB_END here.  */
+      while (BB_END (a) != first && BARRIER_P (BB_END (a)))
+       BB_END (a) = PREV_INSN (BB_END (a));
hunk is all that is needed (and the remaining cfgrtl.c hunks
just a nice to have cleanup, could be postponed for 4.8).
Without this BB_END (a) is a barrier, but deleted one, so get_last_insn ()
was actually moved to some insn before it and therefore when we
emit_insn_after_noloc the b bb unchained sequence, it doesn't match
get_last_insn () when it should.

I'll bootstrap/regtest now just that single hunk, is that ok for
trunk/4.6/4.5?

	Jakub
Richard Biener Feb. 8, 2012, 10:55 a.m. UTC | #2 
On Wed, 8 Feb 2012, Jakub Jelinek wrote:

> On Wed, Feb 08, 2012 at 10:27:42AM +0100, Richard Guenther wrote:
> > Isn't it cheaper to do that before we scan all the sequences?  Thus,
> > I'd expect BLOCK_FOR_INSN to be NULL if the insn is in a sequence?
> > Like simply
> 
> Actually, on a third look, the emit-rtl.c changes aren't needed,
> apparently NEXT_INSN even in cfglayout mode is NULL for the in bb
> insns only if it is equal to get_last_insn (), unless in the header/footer
> sequences, but for those one shouldn't call remove_insn.
> 
> To fix this bug the
> +      /* The above might add a BARRIER as BB_END, but as barriers
> +        aren't valid parts of a bb, remove_insn doesn't update
> +        BB_END if it is a barrier.  So adjust BB_END here.  */
> +      while (BB_END (a) != first && BARRIER_P (BB_END (a)))
> +       BB_END (a) = PREV_INSN (BB_END (a));
> hunk is all that is needed (and the remaining cfgrtl.c hunks
> just a nice to have cleanup, could be postponed for 4.8).
> Without this BB_END (a) is a barrier, but deleted one, so get_last_insn ()
> was actually moved to some insn before it and therefore when we
> emit_insn_after_noloc the b bb unchained sequence, it doesn't match
> get_last_insn () when it should.
> 
> I'll bootstrap/regtest now just that single hunk, is that ok for
> trunk/4.6/4.5?

Yes.

Thanks,
Richard.
diff mbox

Patch

Index: emit-rtl.c
===================================================================
--- emit-rtl.c  (revision 183971)
+++ emit-rtl.c  (working copy)
@@ -3946,7 +3946,7 @@  remove_insn (rtx insn)
     }
   else if (get_last_insn () == insn)
     set_last_insn (prev);
-  else
+  else if (!BLOCK_FOR_INSN (insn))
     {
       struct sequence_stack *stack = seq_stack;
       /* Scan all pending sequences too.  */
@@ -3957,7 +3957,7 @@  remove_insn (rtx insn)
            break;
          }
 
-      gcc_assert (stack);
+      gcc_assert (BARRIER_P (insn) || stack);
     }
   if (!BARRIER_P (insn)
       && (bb = BLOCK_FOR_INSN (insn)))