[PATCH 3/n] ipa: Simplify interface of ipa_call_context::estimate_size_and_time

Jan Hubicka hubicka@ucw.cz
Sat Aug 29 10:38:08 GMT 2020


> Hi,
> 
> this patch changes ipa_call_context::estimate_size_and_time to store
> its results into member fields of the ipa_call_context class instead
> into pointers it receives as parameters so that it can compute ore
> stuff without cluttering the interface even further.
> 
> Bootstrapped and tested on x86_64-linux.  OK for master on top of the
> previous two patches?

ipa_call_context is intended to be structure holding all parameters that
are needed to produce the estimates (size/time/hints).
Adding the actual estimates there would duplicate it with cache.  What
about keeping them separate and inventing ipa_call_estimates structure
to hold the reults?

Honza
> 
> Thanks,
> 
> Martin
> 
> 
> gcc/ChangeLog:
> 
> 2020-08-28  Martin Jambor  <mjambor@suse.cz>
> 
> 	* ipa-fnsummary.h (class ipa_call_context): Changed declaration of
> 	estimate_size_and_time to accept two booleans.  Added an overload
> 	of the method without any parameters.  New fields m_size,
> 	m_min_size, m_time, m_nonspecialized_time and m_hints.
> 	* ipa-cp.c (hint_time_bonus): Changed the second parameter from
> 	just hints to a const reference to ipa_call_context.
> 	(perform_estimation_of_a_value): Adjusted to the new interface of
> 	ipa_call_context::estimate_size_and_time.
> 	* ipa-fnsummary.c (ipa_call_context::estimate_size_and_time):
> 	Modified to store results into member fields of the class.
> 	* ipa-inline-analysis.c (do_estimate_edge_time): Adjusted to the
> 	new interface of ipa_call_context::estimate_size_and_time.
> 	(do_estimate_edge_size): Likewise.
> 	(do_estimate_edge_hints): Likewise.
> ---
>  gcc/ipa-cp.c              | 41 ++++++++++++++++-----------------
>  gcc/ipa-fnsummary.c       | 48 +++++++++++++++++++--------------------
>  gcc/ipa-fnsummary.h       | 33 +++++++++++++++++++++++----
>  gcc/ipa-inline-analysis.c | 45 ++++++++++++++++++------------------
>  4 files changed, 94 insertions(+), 73 deletions(-)
> 
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index e37e71bd24f..010ecfc6b43 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -3182,12 +3182,13 @@ devirtualization_time_bonus (struct cgraph_node *node,
>    return res;
>  }
>  
> -/* Return time bonus incurred because of HINTS.  */
> +/* Return time bonus incurred because of hints stored in CTX.  */
>  
>  static int
> -hint_time_bonus (cgraph_node *node, ipa_hints hints)
> +hint_time_bonus (cgraph_node *node, const ipa_call_context &ctx)
>  {
>    int result = 0;
> +  ipa_hints hints = ctx.m_hints;
>    if (hints & (INLINE_HINT_loop_iterations | INLINE_HINT_loop_stride))
>      result += opt_for_fn (node->decl, param_ipa_cp_loop_hint_bonus);
>    return result;
> @@ -3387,16 +3388,14 @@ perform_estimation_of_a_value (cgraph_node *node, ipa_call_arg_values *avals,
>  			       int removable_params_cost, int est_move_cost,
>  			       ipcp_value_base *val)
>  {
> -  int size, time_benefit;
> -  sreal time, base_time;
> -  ipa_hints hints;
> +  int time_benefit;
>  
>    ipa_call_context ctx = ipa_call_context::for_cloned_node (node, avals);
> -  ctx.estimate_size_and_time (&size, NULL, &time, &base_time, &hints);
> +  ctx.estimate_size_and_time ();
>  
> -  base_time -= time;
> -  if (base_time > 65535)
> -    base_time = 65535;
> +  sreal time_delta = ctx.m_nonspecialized_time - ctx.m_time;
> +  if (time_delta > 65535)
> +    time_delta = 65535;
>  
>    /* Extern inline functions have no cloning local time benefits because they
>       will be inlined anyway.  The only reason to clone them is if it enables
> @@ -3404,11 +3403,12 @@ perform_estimation_of_a_value (cgraph_node *node, ipa_call_arg_values *avals,
>    if (DECL_EXTERNAL (node->decl) && DECL_DECLARED_INLINE_P (node->decl))
>      time_benefit = 0;
>    else
> -    time_benefit = base_time.to_int ()
> +    time_benefit = time_delta.to_int ()
>        + devirtualization_time_bonus (node, avals)
> -      + hint_time_bonus (node, hints)
> +      + hint_time_bonus (node, ctx)
>        + removable_params_cost + est_move_cost;
>  
> +  int size = ctx.m_size;
>    gcc_checking_assert (size >=0);
>    /* The inliner-heuristics based estimates may think that in certain
>       contexts some functions do not have any size at all but we want
> @@ -3463,24 +3463,22 @@ estimate_local_effects (struct cgraph_node *node)
>        || (removable_params_cost && node->can_change_signature))
>      {
>        struct caller_statistics stats;
> -      ipa_hints hints;
> -      sreal time, base_time;
> -      int size;
>  
>        init_caller_stats (&stats);
>        node->call_for_symbol_thunks_and_aliases (gather_caller_stats, &stats,
>  					      false);
>        ipa_call_context ctx = ipa_call_context::for_cloned_node (node, &avals);
> -      ctx.estimate_size_and_time (&size, NULL, &time, &base_time, &hints);
> +      ctx.estimate_size_and_time ();
>  
> -      time -= devirt_bonus;
> -      time -= hint_time_bonus (node, hints);
> -      time -= removable_params_cost;
> -      size -= stats.n_calls * removable_params_cost;
> +      sreal time = ctx.m_nonspecialized_time - ctx.m_time;
> +      time += devirt_bonus;
> +      time += hint_time_bonus (node, ctx);
> +      time += removable_params_cost;
> +      int size = ctx.m_size - stats.n_calls * removable_params_cost;
>  
>        if (dump_file)
>  	fprintf (dump_file, " - context independent values, size: %i, "
> -		 "time_benefit: %f\n", size, (base_time - time).to_double ());
> +		 "time_benefit: %f\n", size, (time).to_double ());
>  
>        if (size <= 0 || node->local)
>  	{
> @@ -3491,8 +3489,7 @@ estimate_local_effects (struct cgraph_node *node)
>  		     "known contexts, code not going to grow.\n");
>  	}
>        else if (good_cloning_opportunity_p (node,
> -					   MIN ((base_time - time).to_int (),
> -						65536),
> +					   MIN ((time).to_int (), 65536),
>  					   stats.freq_sum, stats.count_sum,
>  					   size))
>  	{
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index 2a0f8ad37b2..eb58b143d1c 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -3281,18 +3281,19 @@ ipa_call_context
>  {
>  }
>  
> -/* Estimate size and time needed to execute call in the given context.
> -   Additionally determine hints determined by the context.  Finally compute
> -   minimal size needed for the call that is independent on the call context and
> -   can be used for fast estimates.  Return the values in RET_SIZE,
> -   RET_MIN_SIZE, RET_TIME and RET_HINTS.  */
> +/* Estimate size needed to execute call in the given context and store it to
> +   m_size, compute minimal size needed for the call that is independent on the
> +   call context and can be used for fast estimates and store it to m_min_size.
> +
> +   If EST_TIMES is true, estimate time needed to execute call in the given
> +   context, store it to m_time, and time needed when also calculating things
> +   known to be constant in this context and store it to m_nonspecialized_time.
> +
> +   If EST_HINTS is true, also determine hints for this context and store them
> +   to m_hints.  */
>  
>  void
> -ipa_call_context::estimate_size_and_time (int *ret_size,
> -					  int *ret_min_size,
> -					  sreal *ret_time,
> -					  sreal *ret_nonspecialized_time,
> -					  ipa_hints *ret_hints)
> +ipa_call_context::estimate_size_and_time (bool est_times, bool est_hints)
>  {
>    class ipa_fn_summary *info = ipa_fn_summaries->get (m_node);
>    size_time_entry *e;
> @@ -3322,8 +3323,8 @@ ipa_call_context::estimate_size_and_time (int *ret_size,
>  
>    if (m_node->callees || m_node->indirect_calls)
>      estimate_calls_size_and_time (m_node, &size, &min_size,
> -				  ret_time ? &time : NULL,
> -				  ret_hints ? &hints : NULL, m_possible_truths,
> +				  est_times ? &time : NULL,
> +				  est_hints ? &hints : NULL, m_possible_truths,
>  				  m_avals);
>  
>    sreal nonspecialized_time = time;
> @@ -3350,7 +3351,7 @@ ipa_call_context::estimate_size_and_time (int *ret_size,
>  	     known to be constant in a specialized setting.  */
>  	  if (nonconst)
>  	    size += e->size;
> -	  if (!ret_time)
> +	  if (!est_times)
>  	    continue;
>  	  nonspecialized_time += e->time;
>  	  if (!nonconst)
> @@ -3390,7 +3391,7 @@ ipa_call_context::estimate_size_and_time (int *ret_size,
>    if (time > nonspecialized_time)
>      time = nonspecialized_time;
>  
> -  if (ret_hints)
> +  if (est_hints)
>      {
>        if (info->loop_iterations
>  	  && !info->loop_iterations->evaluate (m_possible_truths))
> @@ -3410,16 +3411,15 @@ ipa_call_context::estimate_size_and_time (int *ret_size,
>    if (dump_file && (dump_flags & TDF_DETAILS))
>      fprintf (dump_file, "\n   size:%i time:%f nonspec time:%f\n", (int) size,
>  	     time.to_double (), nonspecialized_time.to_double ());
> -  if (ret_time)
> -    *ret_time = time;
> -  if (ret_nonspecialized_time)
> -    *ret_nonspecialized_time = nonspecialized_time;
> -  if (ret_size)
> -    *ret_size = size;
> -  if (ret_min_size)
> -    *ret_min_size = min_size;
> -  if (ret_hints)
> -    *ret_hints = hints;
> +  m_size = size;
> +  m_min_size = min_size;
> +  if (est_times)
> +    {
> +      m_time = time;
> +      m_nonspecialized_time = nonspecialized_time;
> +    }
> +  if (est_hints)
> +    m_hints = hints;
>    return;
>  }
>  
> diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h
> index 74a78d31391..6253e375e3b 100644
> --- a/gcc/ipa-fnsummary.h
> +++ b/gcc/ipa-fnsummary.h
> @@ -312,16 +312,41 @@ public:
>    : m_node(NULL)
>    {
>    }
> -  void estimate_size_and_time (int *ret_size, int *ret_min_size,
> -			       sreal *ret_time,
> -			       sreal *ret_nonspecialized_time,
> -			       ipa_hints *ret_hints);
> +  void estimate_size_and_time (bool est_times, bool est_hints);
> +
> +  /* Estimate everything about a call in this context.  */
> +  void estimate_size_and_time ()
> +  {
> +    estimate_size_and_time (true, true);
> +  }
> +
>    void store_to_cache (ipa_node_context_cache_entry *cache) const;
>    bool equivalent_to_p (const ipa_node_context_cache_entry &cache) const;
>    bool exists_p ()
>    {
>      return m_node != NULL;
>    }
> +
> +
> +  /* The following metrics fields only contain valid contents after they have
> +     been calculated by estimate_size_and_time (provided the function was
> +     instructed to compute them).  */
> +
> +  /* Estimated size needed to execute call in the given context. */
> +  int m_size = INT_MIN;
> +  /* Minimal size needed for the call that is independent on the call context
> +     and can be used for fast estimates.  */
> +  int m_min_size = INT_MIN;
> +
> +  /* Estimated time needed to execute call in the given context.  */
> +  sreal m_time;
> +  /* Estimated time needed to execute the function when not ignoring
> +     computations known to be constant in this context.  */
> +  sreal m_nonspecialized_time;
> +
> +  /* Further discovered reasons why to inline or specialize the give calls.  */
> +  ipa_hints m_hints = -1;
> +
>  private:
>    /* Called function.  */
>    cgraph_node *m_node;
> diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
> index 9788ee346ac..706e4fffe4a 100644
> --- a/gcc/ipa-inline-analysis.c
> +++ b/gcc/ipa-inline-analysis.c
> @@ -428,16 +428,10 @@ do_estimate_edge_time (struct cgraph_edge *edge, sreal *ret_nonspec_time)
>  	      && !opt_for_fn (callee->decl, flag_profile_partial_training)
>  	      && !callee->count.ipa_p ())
>  	    {
> -	      sreal chk_time, chk_nonspec_time;
> -	      int chk_size, chk_min_size;
> -
> -	      ipa_hints chk_hints;
> -	      ctx.estimate_size_and_time (&chk_size, &chk_min_size,
> -					  &chk_time, &chk_nonspec_time,
> -					  &chk_hints);
> -	      gcc_assert (chk_size == size && chk_time == time
> -		  	  && chk_nonspec_time == nonspec_time
> -			  && chk_hints == hints);
> +	      ctx.estimate_size_and_time ();
> +	      gcc_assert (ctx.m_size == size && ctx.m_time == time
> +		  	  && ctx.m_nonspecialized_time == nonspec_time
> +			  && ctx.m_hints == hints);
>  	    }
>  	}
>        else
> @@ -450,18 +444,26 @@ do_estimate_edge_time (struct cgraph_edge *edge, sreal *ret_nonspec_time)
>  	  else
>  	    e = node_context_cache->get_create (callee);
>  
> -	  ctx.estimate_size_and_time (&size, &min_size,
> -				      &time, &nonspec_time, &hints);
> +	  ctx.estimate_size_and_time ();
>  	  ctx.store_to_cache (&e->entry);
> +	  size = ctx.m_size;
>  	  e->entry.m_size = size;
> +	  time = ctx.m_time;
>  	  e->entry.m_time = time;
> +	  nonspec_time = ctx.m_nonspecialized_time;
>  	  e->entry.m_nonspec_time = nonspec_time;
> +	  hints = ctx.m_hints;
>  	  e->entry.m_hints = hints;
>  	}
>      }
>    else
> -    ctx.estimate_size_and_time (&size, &min_size,
> -				&time, &nonspec_time, &hints);
> +    {
> +      ctx.estimate_size_and_time ();
> +      size = ctx.m_size;
> +      time = ctx.m_time;
> +      nonspec_time = ctx.m_nonspecialized_time;
> +      hints = ctx.m_hints;
> +    }
>  
>    /* When we have profile feedback, we can quite safely identify hot
>       edges and for those we disable size limits.  Don't do that when
> @@ -524,7 +526,6 @@ ipa_remove_from_growth_caches (struct cgraph_edge *edge)
>  int
>  do_estimate_edge_size (struct cgraph_edge *edge)
>  {
> -  int size;
>    ipa_call_arg_values avals;
>  
>    /* When we do caching, use do_estimate_edge_time to populate the entry.  */
> @@ -532,7 +533,7 @@ do_estimate_edge_size (struct cgraph_edge *edge)
>    if (edge_growth_cache != NULL)
>      {
>        do_estimate_edge_time (edge);
> -      size = edge_growth_cache->get (edge)->size;
> +      int size = edge_growth_cache->get (edge)->size;
>        gcc_checking_assert (size);
>        return size - (size > 0);
>      }
> @@ -541,8 +542,8 @@ do_estimate_edge_size (struct cgraph_edge *edge)
>    gcc_checking_assert (edge->inline_failed);
>    ipa_call_context ctx = ipa_call_context::for_inlined_edge (edge, vNULL,
>  							     &avals);
> -  ctx.estimate_size_and_time (&size, NULL, NULL, NULL, NULL);
> -  return size;
> +  ctx.estimate_size_and_time (false, false);
> +  return ctx.m_size;
>  }
>  
>  
> @@ -552,7 +553,6 @@ do_estimate_edge_size (struct cgraph_edge *edge)
>  ipa_hints
>  do_estimate_edge_hints (struct cgraph_edge *edge)
>  {
> -  ipa_hints hints;
>    ipa_call_arg_values avals;
>  
>    /* When we do caching, use do_estimate_edge_time to populate the entry.  */
> @@ -560,7 +560,7 @@ do_estimate_edge_hints (struct cgraph_edge *edge)
>    if (edge_growth_cache != NULL)
>      {
>        do_estimate_edge_time (edge);
> -      hints = edge_growth_cache->get (edge)->hints;
> +      ipa_hints hints = edge_growth_cache->get (edge)->hints;
>        gcc_checking_assert (hints);
>        return hints - 1;
>      }
> @@ -569,9 +569,8 @@ do_estimate_edge_hints (struct cgraph_edge *edge)
>    gcc_checking_assert (edge->inline_failed);
>    ipa_call_context ctx = ipa_call_context::for_inlined_edge (edge, vNULL,
>  							     &avals);
> -  ctx.estimate_size_and_time (NULL, NULL, NULL, NULL, &hints);
> -  hints |= simple_edge_hints (edge);
> -  return hints;
> +  ctx.estimate_size_and_time (false, true);
> +  return ctx.m_hints | simple_edge_hints (edge);
>  }
>  
>  /* Estimate the size of NODE after inlining EDGE which should be an
> -- 
> 2.28.0
> 


More information about the Gcc-patches mailing list