| Message ID | 20101115202108.GE29412@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
> 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.
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
--- 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:; +}