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 <ja...@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
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)))
? 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
>
>
--
Richard Guenther <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
