Message ID | 20150105170909.GV1667@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
> Hi! > > The early inliner ICEs on the following testcase, because redirect_all_calls > replaces a call to a function that could throw with __builtin_unreachable > (and, note, it still has the bug that it doesn't drop the function > arguments), but nothing cleans up the EH stuff for that change. > > During function versioning fixup_cfg pass is supposed to handle that, > during IPA inlining there is explicit call to execute_fixup_cfg, but during > early inlining there is not. I am still confused why early inliner does any redirections? It should not. Edge redirection is part of the IPA optimization machinery. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > ok for trunk? > > Honza, will you take care of dropping the arguments when you redirect to > a function that takes no arguments (I believe only __builtin_unreachable > and __pure_virtual are the cases where we do that). Yep, i will try to find time for that after arriving to Calgary this weekend. There are quite few places where substitution can happen, so I need to check them curefuly. Honza > > 2015-01-05 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/64465 > * tree-inline.c (redirect_all_calls): During inlining > clean up EH stmts and EH edges if redirect_call_stmt_to_callee > changed the stmt to a non-throwing call. > > * gcc.dg/pr64465.c: New test. > > --- gcc/tree-inline.c.jj 2015-01-05 13:07:15.186507607 +0100 > +++ gcc/tree-inline.c 2015-01-05 15:30:33.715001491 +0100 > @@ -2582,13 +2582,19 @@ void > redirect_all_calls (copy_body_data * id, basic_block bb) > { > gimple_stmt_iterator si; > + gimple last = last_stmt (bb); > for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si)) > { > - if (is_gimple_call (gsi_stmt (si))) > + gimple stmt = gsi_stmt (si); > + if (is_gimple_call (stmt)) > { > - struct cgraph_edge *edge = id->dst_node->get_edge (gsi_stmt (si)); > + struct cgraph_edge *edge = id->dst_node->get_edge (stmt); > if (edge) > - edge->redirect_call_stmt_to_callee (); > + { > + edge->redirect_call_stmt_to_callee (); > + if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) > + gimple_purge_dead_eh_edges (bb); > + } > } > } > } > --- gcc/testsuite/gcc.dg/pr64465.c.jj 2015-01-05 15:06:31.369183163 +0100 > +++ gcc/testsuite/gcc.dg/pr64465.c 2015-01-05 15:06:31.369183163 +0100 > @@ -0,0 +1,22 @@ > +/* PR tree-optimization/64465 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fexceptions" } */ > + > +extern int foo (int *); > +extern int bar (int, int); > +static inline __attribute__ ((__always_inline__)) > +int baz (int o) > +{ > + if (__builtin_constant_p (o)) > + return bar (o, 1); > + return bar (o, 0); > +} > + > +void > +test (void) > +{ > + int s; > + foo (&s); > + baz (4); > + baz (s); > +} > > Jakub
On January 5, 2015 6:37:20 PM CET, Jan Hubicka <hubicka@ucw.cz> wrote: >> Hi! >> >> The early inliner ICEs on the following testcase, because >redirect_all_calls >> replaces a call to a function that could throw with >__builtin_unreachable >> (and, note, it still has the bug that it doesn't drop the function >> arguments), but nothing cleans up the EH stuff for that change. >> >> During function versioning fixup_cfg pass is supposed to handle that, >> during IPA inlining there is explicit call to execute_fixup_cfg, but >during >> early inlining there is not. > >I am still confused why early inliner does any redirections? It should >not. >Edge redirection is part of the IPA optimization machinery. Possibly through stmt folding. Richard. >> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, >> ok for trunk? >> >> Honza, will you take care of dropping the arguments when you redirect >to >> a function that takes no arguments (I believe only >__builtin_unreachable >> and __pure_virtual are the cases where we do that). > >Yep, i will try to find time for that after arriving to Calgary this >weekend. >There are quite few places where substitution can happen, so I need to >check >them curefuly. > >Honza >> >> 2015-01-05 Jakub Jelinek <jakub@redhat.com> >> >> PR tree-optimization/64465 >> * tree-inline.c (redirect_all_calls): During inlining >> clean up EH stmts and EH edges if redirect_call_stmt_to_callee >> changed the stmt to a non-throwing call. >> >> * gcc.dg/pr64465.c: New test. >> >> --- gcc/tree-inline.c.jj 2015-01-05 13:07:15.186507607 +0100 >> +++ gcc/tree-inline.c 2015-01-05 15:30:33.715001491 +0100 >> @@ -2582,13 +2582,19 @@ void >> redirect_all_calls (copy_body_data * id, basic_block bb) >> { >> gimple_stmt_iterator si; >> + gimple last = last_stmt (bb); >> for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si)) >> { >> - if (is_gimple_call (gsi_stmt (si))) >> + gimple stmt = gsi_stmt (si); >> + if (is_gimple_call (stmt)) >> { >> - struct cgraph_edge *edge = id->dst_node->get_edge (gsi_stmt >(si)); >> + struct cgraph_edge *edge = id->dst_node->get_edge (stmt); >> if (edge) >> - edge->redirect_call_stmt_to_callee (); >> + { >> + edge->redirect_call_stmt_to_callee (); >> + if (stmt == last && id->call_stmt && maybe_clean_eh_stmt >(stmt)) >> + gimple_purge_dead_eh_edges (bb); >> + } >> } >> } >> } >> --- gcc/testsuite/gcc.dg/pr64465.c.jj 2015-01-05 15:06:31.369183163 >+0100 >> +++ gcc/testsuite/gcc.dg/pr64465.c 2015-01-05 15:06:31.369183163 >+0100 >> @@ -0,0 +1,22 @@ >> +/* PR tree-optimization/64465 */ >> +/* { dg-do compile } */ >> +/* { dg-options "-O2 -fexceptions" } */ >> + >> +extern int foo (int *); >> +extern int bar (int, int); >> +static inline __attribute__ ((__always_inline__)) >> +int baz (int o) >> +{ >> + if (__builtin_constant_p (o)) >> + return bar (o, 1); >> + return bar (o, 0); >> +} >> + >> +void >> +test (void) >> +{ >> + int s; >> + foo (&s); >> + baz (4); >> + baz (s); >> +} >> >> Jakub
On Mon, Jan 05, 2015 at 07:38:20PM +0100, Richard Biener wrote: > >> During function versioning fixup_cfg pass is supposed to handle that, > >> during IPA inlining there is explicit call to execute_fixup_cfg, but > >during > >> early inlining there is not. > > > >I am still confused why early inliner does any redirections? It should > >not. > >Edge redirection is part of the IPA optimization machinery. > > Possibly through stmt folding. Generally yes, in this case, I doubt it, it isn't a builtin function. But it is guarded by a false condition (__builtin_constant_p test), so supposedly the early inliner figures out that it doesn't need to bother with the dead call and replaces it with __builtin_unreachable anyway. Jakub
> On Mon, Jan 05, 2015 at 07:38:20PM +0100, Richard Biener wrote: > > >> During function versioning fixup_cfg pass is supposed to handle that, > > >> during IPA inlining there is explicit call to execute_fixup_cfg, but > > >during > > >> early inlining there is not. > > > > > >I am still confused why early inliner does any redirections? It should > > >not. > > >Edge redirection is part of the IPA optimization machinery. > > > > Possibly through stmt folding. > > Generally yes, in this case, I doubt it, it isn't a builtin function. > But it is guarded by a false condition (__builtin_constant_p test), so > supposedly the early inliner figures out that it doesn't need to bother > with the dead call and replaces it with __builtin_unreachable anyway. Ah, I see, this is resonable explanation - this one changed with enabling the predicates early. The patch is OK. Thanks. Honza > > Jakub
--- gcc/tree-inline.c.jj 2015-01-05 13:07:15.186507607 +0100 +++ gcc/tree-inline.c 2015-01-05 15:30:33.715001491 +0100 @@ -2582,13 +2582,19 @@ void redirect_all_calls (copy_body_data * id, basic_block bb) { gimple_stmt_iterator si; + gimple last = last_stmt (bb); for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si)) { - if (is_gimple_call (gsi_stmt (si))) + gimple stmt = gsi_stmt (si); + if (is_gimple_call (stmt)) { - struct cgraph_edge *edge = id->dst_node->get_edge (gsi_stmt (si)); + struct cgraph_edge *edge = id->dst_node->get_edge (stmt); if (edge) - edge->redirect_call_stmt_to_callee (); + { + edge->redirect_call_stmt_to_callee (); + if (stmt == last && id->call_stmt && maybe_clean_eh_stmt (stmt)) + gimple_purge_dead_eh_edges (bb); + } } } } --- gcc/testsuite/gcc.dg/pr64465.c.jj 2015-01-05 15:06:31.369183163 +0100 +++ gcc/testsuite/gcc.dg/pr64465.c 2015-01-05 15:06:31.369183163 +0100 @@ -0,0 +1,22 @@ +/* PR tree-optimization/64465 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fexceptions" } */ + +extern int foo (int *); +extern int bar (int, int); +static inline __attribute__ ((__always_inline__)) +int baz (int o) +{ + if (__builtin_constant_p (o)) + return bar (o, 1); + return bar (o, 0); +} + +void +test (void) +{ + int s; + foo (&s); + baz (4); + baz (s); +}