Patchwork Fix combiner cfglayout issue (PR rtl-optimization/46440)

login
register
mail settings
Submitter Jakub Jelinek
Date Nov. 15, 2010, 8:21 p.m.
Message ID <20101115202108.GE29412@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/71281/
State New
Headers show

Comments

Jakub Jelinek - Nov. 15, 2010, 8:21 p.m.
Hi!

On this testcase we ICE during flow checking, because
combiner replaced an indirect jump (which was before going into
cfglayout mode followed by BARRIER, which stays in the footer)
by direct unconditional jump.  Fixed by removing the BARRIER
from the footer in that case.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Alternatively, perhaps the current_ir_type () == IR_RTL_CFGLAYOUT test
can be assumed to be 1.

2010-11-15  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/46440
	* combine.c (update_cfg_for_uncondjump): When changing
	an indirect jump into unconditional jump, remove BARRIERs
	from bb's footer.

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


	Jakub
Paolo Bonzini - Nov. 15, 2010, 8:40 p.m.
On 11/15/2010 09:21 PM, Jakub Jelinek wrote:
> Hi!
>
> On this testcase we ICE during flow checking, because
> combiner replaced an indirect jump (which was before going into
> cfglayout mode followed by BARRIER, which stays in the footer)
> by direct unconditional jump.  Fixed by removing the BARRIER
> from the footer in that case.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Alternatively, perhaps the current_ir_type () == IR_RTL_CFGLAYOUT test
> can be assumed to be 1.

Yes, in the end update_cfg_for_uncondjump is already assuming cfglayout 
mode.

I'm wondering what happens if you remove the BARRIER altogether in 
skip_insns_after_block.  If it's just a matter of saving some garbage, 
it's not that much since we're going into/out of cfglayout mode 2-4 
times per function.

Paolo
Jakub Jelinek - Nov. 15, 2010, 8:53 p.m.
On Mon, Nov 15, 2010 at 09:40:18PM +0100, Paolo Bonzini wrote:
> On 11/15/2010 09:21 PM, Jakub Jelinek wrote:
> >Alternatively, perhaps the current_ir_type () == IR_RTL_CFGLAYOUT test
> >can be assumed to be 1.
> 
> Yes, in the end update_cfg_for_uncondjump is already assuming
> cfglayout mode.

If that is the right version, consider it done.

> I'm wondering what happens if you remove the BARRIER altogether in
> skip_insns_after_block.  If it's just a matter of saving some
> garbage, it's not that much since we're going into/out of cfglayout
> mode 2-4 times per function.

Yeah, I believe the intent of the il.rtl->footer and BARRIERs being moved
there was to save garbage, in some functions there could be many barriers
and if we remove them resp. readd them each time we go into cfglayout mode
resp. out of it, it could be measurable.

Perhaps we could nuke the barriers somewhere when going out of cfglayout
mode if we see the bb has some fallthru edge.  Just currently (not sure why)
it has been the duty of each of the passes to handle that stuff on its own.

	Jakub
Paolo Bonzini - Nov. 15, 2010, 11:10 p.m.
On 11/15/2010 09:53 PM, Jakub Jelinek wrote:
> Yeah, I believe the intent of the il.rtl->footer and BARRIERs being moved
> there was to save garbage, in some functions there could be many barriers
> and if we remove them resp. readd them each time we go into cfglayout mode
> resp. out of it, it could be measurable.

160/320 (32-bit/64-bit) bytes per BB across a whole compilation...

Paolo
Eric Botcazou - Nov. 17, 2010, 12:19 p.m.
> 2010-11-15  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR rtl-optimization/46440
> 	* combine.c (update_cfg_for_uncondjump): When changing
> 	an indirect jump into unconditional jump, remove BARRIERs
> 	from bb's footer.
>
> 	* gcc.dg/pr46440.c: New test.

OK without the IR test, thanks.
Jeff Law - Nov. 18, 2010, 12:49 a.m.
On 11/15/10 13:21, Jakub Jelinek wrote:
> Hi!
>
> On this testcase we ICE during flow checking, because
> combiner replaced an indirect jump (which was before going into
> cfglayout mode followed by BARRIER, which stays in the footer)
> by direct unconditional jump.  Fixed by removing the BARRIER
> from the footer in that case.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Alternatively, perhaps the current_ir_type () == IR_RTL_CFGLAYOUT test
> can be assumed to be 1.
>
> 2010-11-15  Jakub Jelinek<jakub@redhat.com>
>
> 	PR rtl-optimization/46440
> 	* combine.c (update_cfg_for_uncondjump): When changing
> 	an indirect jump into unconditional jump, remove BARRIERs
> 	from bb's footer.
>
> 	* gcc.dg/pr46440.c: New test.
No strong opinion on the current_ir_type test -- 
update_cfg_for_uncondjump isn't used outside of combine right now, so 
you can probably make assumptions about the current IR; but if we ever 
pulled it out to use elsewhere then the assumption might not hold.

OK with or without the test -- your call.  Perhaps an assert instead?


jeff

Patch

--- gcc/combine.c.jj	2010-11-09 13:58:30.000000000 +0100
+++ gcc/combine.c	2010-11-15 13:53:13.000000000 +0100
@@ -2460,7 +2460,28 @@  update_cfg_for_uncondjump (rtx insn)
 
   delete_insn (insn);
   if (at_end && EDGE_COUNT (bb->succs) == 1)
-    single_succ_edge (bb)->flags |= EDGE_FALLTHRU;
+    {
+      single_succ_edge (bb)->flags |= EDGE_FALLTHRU;
+
+      if (current_ir_type () == IR_RTL_CFGLAYOUT)
+	{
+	  rtx insn;
+
+	  /* Remove barriers from the footer if there are any.  */
+	  for (insn = bb->il.rtl->footer; insn; insn = NEXT_INSN (insn))
+	    if (BARRIER_P (insn))
+	      {
+		if (PREV_INSN (insn))
+		  NEXT_INSN (PREV_INSN (insn)) = NEXT_INSN (insn);
+		else
+		  bb->il.rtl->footer = NEXT_INSN (insn);
+		if (NEXT_INSN (insn))
+		  PREV_INSN (NEXT_INSN (insn)) = PREV_INSN (insn);
+	      }
+	    else if (LABEL_P (insn))
+	      break;
+	}
+    }
 }
 
 /* Try to combine the insns I0, I1 and I2 into I3.
--- gcc/testsuite/gcc.dg/pr46440.c.jj	2010-11-15 14:12:53.000000000 +0100
+++ gcc/testsuite/gcc.dg/pr46440.c	2010-11-15 14:12:45.000000000 +0100
@@ -0,0 +1,25 @@ 
+/* PR rtl-optimization/46440 */
+/* { dg-do compile } */
+/* { dg-options "-O -fstack-protector -fno-tree-dominator-opts -fno-tree-fre" } */
+/* { dg-require-effective-target fstack_protector } */
+
+int i;
+
+void bar (char *);
+
+void
+foo (void)
+{
+  void *l;
+  char c[64];
+  bar (c);
+  i = 1;
+  if (i)
+    l = &&l1;
+  else
+    l = &&l2;
+  goto *l;
+l2:
+  __builtin_abort ();
+l1:;
+}