Re: [PATCH] Towards fixing PR47663

[Jan Hubicka <hubicka at ucw dot cz>]

> 2011-02-16  Richard Guenther  <rguenther@suse.de>
> 
> 	* ipa-inline.c (cgraph_estimate_edge_growth): New function.
> 	(cgraph_estimate_growth): Use it.
> 	(cgraph_edge_badness): Likewise.
> 	(cgraph_check_inline_limits): Take an edge argument.
> 	(cgraph_decide_inlining_of_small_functions): Adjust.
> 	(cgraph_decide_inlining): Likewise.

This is OK.

>   static inline int
>   cgraph_estimate_edge_growth (struct cgraph_edge *edge)
>   {
> +   int call_stmt_size;
> +   /* ???  We throw away cgraph edges all the time so the information
> +      we store in edges doesn't persist for early inlining.  Ugh.  */
> +   if (!edge->call_stmt)
> +     call_stmt_size = edge->call_stmt_size;
> +   else
> +     call_stmt_size = estimate_num_insns (edge->call_stmt, &eni_size_weights);

This should go away with the Alexandre's changes (I will look into it tonight)
> *************** lookup_recursive_calls (struct cgraph_no
> *** 826,843 ****
>      is NULL.  */
>   
>   static bool
> ! cgraph_decide_recursive_inlining (struct cgraph_node *node,
>   				  VEC (cgraph_edge_p, heap) **new_edges)
>   {
>     int limit = PARAM_VALUE (PARAM_MAX_INLINE_INSNS_RECURSIVE_AUTO);
>     int max_depth = PARAM_VALUE (PARAM_MAX_INLINE_RECURSIVE_DEPTH_AUTO);
>     int probability = PARAM_VALUE (PARAM_MIN_INLINE_RECURSIVE_PROBABILITY);
>     fibheap_t heap;
>     struct cgraph_edge *e;
>     struct cgraph_node *master_clone, *next;
>     int depth = 0;
>     int n = 0;
>   
>     /* It does not make sense to recursively inline always-inline functions
>        as we are going to sorry() on the remaining calls anyway.  */
>     if (node->local.disregard_inline_limits
> --- 812,834 ----
>      is NULL.  */
>   
>   static bool
> ! cgraph_decide_recursive_inlining (struct cgraph_edge *edge,
>   				  VEC (cgraph_edge_p, heap) **new_edges)
>   {
>     int limit = PARAM_VALUE (PARAM_MAX_INLINE_INSNS_RECURSIVE_AUTO);
>     int max_depth = PARAM_VALUE (PARAM_MAX_INLINE_RECURSIVE_DEPTH_AUTO);
>     int probability = PARAM_VALUE (PARAM_MIN_INLINE_RECURSIVE_PROBABILITY);
>     fibheap_t heap;
> +   struct cgraph_node *node;
>     struct cgraph_edge *e;
>     struct cgraph_node *master_clone, *next;
>     int depth = 0;
>     int n = 0;
>   
> +   node = edge->caller;
> +   if (node->global.inlined_to)
> +     node = node->global.inlined_to;
> + 

You should not need this - incremental inliner is run on inline candidates and those
are not functions already inlined.
> --- 1148,1164 ----
>    	not_good = CIF_OPTIMIZING_FOR_SIZE;
>         if (not_good && growth > 0 && cgraph_estimate_growth (edge->callee) > 0)
>   	{
> ! 	  edge->inline_failed = not_good;
> ! 	  if (dump_file)
> ! 	    fprintf (dump_file, " inline_failed:%s.\n",
> ! 		     cgraph_inline_failed_string (edge->inline_failed));
>   	  continue;
>   	}
>         if (!cgraph_default_inline_p (edge->callee, &edge->inline_failed))
>   	{
> ! 	  if (dump_file)
> ! 	    fprintf (dump_file, " inline_failed:%s.\n",
> ! 		     cgraph_inline_failed_string (edge->inline_failed));
>   	  continue;
>   	}
>         if (!tree_can_inline_p (edge)

The main inliner makes bit lousy decision when it starts inlining recursive edges (as it always
inline the whole function into itself).  Any particular reason you droppped the recursive
inlining check here?
> *************** cgraph_flatten (struct cgraph_node *node
> *** 1359,1365 ****
>   	  continue;
>   	}
>   
> !       if (cgraph_recursive_inlining_p (node, e->callee, &e->inline_failed))
>   	{
>   	  if (dump_file)
>   	    fprintf (dump_file, "Not inlining: recursive call.\n");
> --- 1340,1346 ----
>   	  continue;
>   	}
>   
> !       if (cgraph_edge_recursive_p (e))
>   	{
>   	  if (dump_file)
>   	    fprintf (dump_file, "Not inlining: recursive call.\n");

I would actually still preffer to have CIF reason set correctly when we don't inline because
edge is recursive - even if you decide to not warn on this case, it is useful for debugging.
I would also still warn. Recursive inline functions are kind of insane and they can be mutually
recursive or result of similar not completely trivial pattern.
But I have no strict opinion on this.
> *************** compute_inline_parameters (struct cgraph
> *** 2011,2016 ****
> --- 1967,1979 ----
>       node->local.disregard_inline_limits
>         = DECL_DISREGARD_INLINE_LIMITS (node->decl);
>     estimate_function_body_sizes (node);
> +   /* Compute size of call statements.  We have to do this for callers here,
> +      those sizes need to be present for edges _to_ us as early as
> +      we are finished with early opts.
> +      ???  Possibly need the same for _time.  */
Yes, especially after you dropped the logic computing it above.

With those changes the patch seems fine.
> Index: gcc/tree-inline.c
> ===================================================================
> *** gcc/tree-inline.c.orig	2011-02-15 15:41:21.000000000 +0100
> --- gcc/tree-inline.c	2011-02-16 14:29:43.000000000 +0100
> *************** estimate_num_insns (gimple stmt, eni_wei
> *** 3519,3525 ****
>   	if (decl)
>   	  funtype = TREE_TYPE (decl);
>   
> ! 	if (!VOID_TYPE_P (TREE_TYPE (funtype)))
>   	  cost += estimate_move_cost (TREE_TYPE (funtype));
>   
>   	if (funtype)
> --- 3519,3526 ----
>   	if (decl)
>   	  funtype = TREE_TYPE (decl);
>   
> ! 	if (!VOID_TYPE_P (TREE_TYPE (funtype))
> ! 	    && gimple_call_lhs (stmt))

Are we consistent on DCE removing LHS of call statements whose result is unused?
I remember there was some back and forth games with this.

Honza