Fix einline ICE with EH (PR tree-optimization/64465)

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.

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).

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.


	Jakub

> 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