Re: [PATCH] Fix cfg_layout_merge_blocks (PR rtl-optimization/52139)

2012-02-08 Thread Richard Guenther 
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.c2012-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

Re: [PATCH] Fix cfg_layout_merge_blocks (PR rtl-optimization/52139)

2012-02-08 Thread Jakub Jelinek 
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

Re: [PATCH] Fix cfg_layout_merge_blocks (PR rtl-optimization/52139)

2012-02-08 Thread Richard Guenther 
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.

Re: [PATCH] Fix cfg_layout_merge_blocks (PR rtl-optimization/52139)

2012-02-08 Thread Jakub Jelinek 
On Wed, Feb 08, 2012 at 11:55:52AM +0100, Richard Guenther wrote:
  I'll bootstrap/regtest now just that single hunk, is that ok for
  trunk/4.6/4.5?
 
 Yes.

Thanks, this is what I've committed after bootstrap/regtest on x86_64-linux
and i686-linux:

2012-02-08  Jakub Jelinek  ja...@redhat.com

PR rtl-optimization/52139
* 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.

* gcc.dg/pr52139.c: New test.

--- gcc/cfgrtl.c.jj 2012-02-07 16:05:47.977533716 +0100
+++ gcc/cfgrtl.c2012-02-07 17:03:52.925956927 +0100
@@ -2871,6 +2871,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;
 }
--- 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

[PATCH] Fix cfg_layout_merge_blocks (PR rtl-optimization/52139)

2012-02-07 Thread Jakub Jelinek 
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);
+   }
 }
   if (!BARRIER_P (insn)
(bb = BLOCK_FOR_INSN (insn)))
--- gcc/cfgrtl.c.jj 2012-02-07 16:05:47.977533716 +0100
+++ gcc/cfgrtl.c2012-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. */
+