Re: [PATCH] Clean up early inliner

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Wed, Apr 07, 2010 at 11:31:03AM +0200, Richard Guenther wrote:
> 
> This cleans up the early inliner, simplifying it and making it the
> one that resolves all always-inline inlines.  It also should remove
> the need to run the IPA inliner when not optimizing or not inlining.
> 
> The single early inlining cleanup that is left is the interaction
> with profiling.
> 
> Boostrap and regtest running on x86_64-unknown-linux-gnu, ok for
> trunk?
> 
> Thanks,
> Richard.
> 
> 2010-01-29  Richard Guenther  <rguenther@suse.de>
> 
> 	* ipa.c (cgraph_postorder): Adjust postorder to guarantee
> 	single-iteration always-inline inlining.
> 	* ipa-inline.c (cgraph_mark_inline): Do not return anything.
> 	(cgraph_decide_inlining): Do not handle always-inline
> 	specially.
> 	(try_inline): Remove always-inline cycle detection special case.
> 	Do not recurse on always-inlines.
> 	(cgraph_early_inlining): Do not iterate if not optimizing.
> 	(cgraph_gate_early_inlining): remove.
> 	(pass_early_inline): Run unconditionally.
> 	(gate_cgraph_decide_inlining): New function.
> 	(pass_ipa_inline): Use it.  Do not run the IPA inliner if
> 	not inlining or optimizing.
> 
> 	* gcc.dg/torture/inline-2.c: New testcase.
> 

...

> *************** cgraph_decide_inlining_of_small_function
> *** 1127,1132 ****
> --- 1124,1211 ----
>     BITMAP_FREE (updated_nodes);
>   }
>   
> + /* Flatten NODE from the IPA inliner.  */
> + 
> + static void
> + cgraph_flatten (struct cgraph_node *node)
> + {
> +   struct cgraph_edge *e;
> +   void *old_mode;
> + 
> +   old_mode = node->aux;

I believe that the code below (the test of callee_node) assumes that
all unprocessed nodes have node->aux NULL and after having briefly
glanced at the call sites I tend to think it is correct.  Therefore I
would consider asserting that and setting it back to NULL at the end
of the function.

> + 
> +   node->aux = (void *)(size_t) INLINE_ALL;
> + 
> +   for (e = node->callees; e; e = e->next_callee)
> +     {
> +       struct cgraph_node *callee = e->callee;

I find it quite confusing that throughout this cycle we refer to the
same node as both callee and e->callee in a rather random fashion.
Wouldn't we be better off without this variable?

> +       void *callee_mode = callee->aux;
> + 
> +       if (e->call_stmt_cannot_inline_p)
> + 	continue;
> + 
> +       /* We've hit cycle?  It is time to give up.  */
> +       if (callee_mode)
> + 	{
> + 	  if (dump_file)
> + 	    fprintf (dump_file,
> + 		     "Not inlining %s into %s to avoid cycle.\n",
> + 		     cgraph_node_name (callee),
> + 		     cgraph_node_name (e->caller));
> + 	  e->inline_failed = (e->callee->local.disregard_inline_limits
> + 			      ? CIF_RECURSIVE_INLINING : CIF_UNSPECIFIED);
> + 	  continue;
> + 	}
> + 
> +       /* When the edge is already inlined, we just need to recurse into
> + 	 it in order to fully flatten the leaves.  */
> +       if (!e->inline_failed)
> + 	{
> + 	  callee->aux = (void *)(size_t) INLINE_ALL;
> + 	  cgraph_flatten (e->callee);
> + 	  callee->aux = callee_mode;

Aren't these assignments to callee->aux redundant?  It seems that
cgraph_flatten does the very same thing.

> + 	  continue;
> + 	}
> + 
> +       if (cgraph_recursive_inlining_p (node, e->callee, &e->inline_failed))
> + 	{
> + 	  if (dump_file)
> + 	    fprintf (dump_file, "Not inlining: recursive call.\n");
> + 	  continue;
> + 	}
> +       if (!tree_can_inline_p (e))
> + 	{
> + 	  if (dump_file)
> + 	    fprintf (dump_file, "Not inlining: %s",
> + 		     cgraph_inline_failed_string (e->inline_failed));
> + 	  continue;
> + 	}
> +       if (!e->callee->analyzed)
> + 	{
> + 	  if (dump_file)
> + 	    fprintf (dump_file,
> + 		     "Not inlining: Function body no longer available.\n");
> + 	  continue;
> + 	}
> + 
> +       if (dump_file)
> + 	fprintf (dump_file, " Inlining %s into %s.\n",
> + 		 cgraph_node_name (e->callee),
> + 		 cgraph_node_name (e->caller));
> +       if (e->inline_failed)
> + 	{
> + 	  cgraph_mark_inline (e);
> + 
> + 	  /* Flattening needs to be done recursively.  */
> + 	  callee->aux = (void *)(size_t) INLINE_ALL;
> + 	  cgraph_flatten (e->callee);
> + 	  callee->aux = callee_mode;

Likewise.

Thanks,

Martin

> + 	}
> +     }
> + 
> +   node->aux = old_mode;
> + }
> + 
>   /* Decide on the inlining.  We do so in the topological order to avoid
>      expenses on updating data structures.  */
>