This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Indirect-call topn targets profiler (instrumentation)
- From: Rong Xu <xur at google dot com>
- To: Jan Hubicka <hubicka at ucw dot cz>
- Cc: Xinliang David Li <davidxl at google dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 6 Oct 2014 13:11:57 -0700
- Subject: Re: [PATCH] Indirect-call topn targets profiler (instrumentation)
- Authentication-results: sourceware.org; auth=none
- References: <CAF1bQ=Q-RtdZN5-YSHcYqNc0_dG7NeFTOo59ndPT+qa+EkTKKQ at mail dot gmail dot com> <20141006182239 dot GB6942 at kam dot mff dot cuni dot cz> <CAAkRFZJgRFgSaKYx_=s8MppJyZysShEP1j7LZSPp92tkm6XU_g at mail dot gmail dot com> <20141006192101 dot GB12741 at atrey dot karlin dot mff dot cuni dot cz>
On Mon, Oct 6, 2014 at 12:21 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Mon, Oct 6, 2014 at 11:22 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Rong,
>> > Would be possible to use topn profiler to get resonale histograms for switch
>> > expansion, too? In that case it may make sense to have value version of it as well.
>>
Yes. That's doable. I'm just not sure if top 2 entries are enough. I
can see from your example that it will be useful for BBs having
multiple case stmt.
>> The underlying topn_profiler can be used the same way as the current
>> one_value profiler for different scenarios. However, I wonder if it is
>> useful for switch value profiling -- the arc profiling can do the
>> same.
>
> Well, for switch expansion one really wants a histogram of values across the switch
> values for the decision tree balancing. But I guess this profiler does not fit
> that very well.
>
> Consider cases like
>
> switch (a)
> {
> case 3:
> case 5:
> case 7:
> prime();
> case 2:
> case 4:
> case 6:
> even();
> default:
> bogus ();
> }
>
> Arc profiling does tell you how often a is prime or even, but it does not help you
> to balance the tree, because the value ranges are iinterlacing
>>
>> David
>>
>> >
>> > Otherwise the patch seems OK. I would implement it myself by introducing separate
>> > variables holding the topn profiler declaration, but can not think of downsides of
>> > doing it your way.
>> >
>> > The topn profiler should be better on determining the dominating target, so perhaps
>> > you could look into hooking it into ipa-profile.c. Extending speculative edges for
>> > multiple targets ought to be also quite easy - we need to update speculative_call_info
>> > to collect edges annotated to a given call stmt into a vector instead of taking the
>> > two references it does now.
>> >
>> > It would be also very nice to update it to handle case where all the edges are direct
>> > so we can use it tospeculatively inlinine functions that can be interposed at runtime.
Yes. I'm looking at this too. I actually ported the fdo-use part of
the patch and it passes spec2006. But it performs the transformation
using the old way (in gimple_ic_transform()) -- so I did not send out
that patch.
I'm trying to do this in the new way (in ipa-profile()). In any case,
that will be a separated patch.
Also, the switch expansion profile should be also a separated patch.
Is it ok to commit these two patches now?
Thanks,
-Rong
>> >
>> > Thanks,
>> > Honza
>> >
>> > Index: gcc/tree-profile.c
>> > ===================================================================
>> > --- gcc/tree-profile.c (revision 215886)
>> > +++ gcc/tree-profile.c (working copy)
>> > @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. If not see
>> > #include "target.h"
>> > #include "tree-cfgcleanup.h"
>> > #include "tree-nested.h"
>> > +#include "params.h"
>> >
>> > static GTY(()) tree gcov_type_node;
>> > static GTY(()) tree tree_interval_profiler_fn;
>> > @@ -101,7 +102,10 @@ init_ic_make_global_vars (void)
>> > {
>> > ic_void_ptr_var
>> > = build_decl (UNKNOWN_LOCATION, VAR_DECL,
>> > - get_identifier ("__gcov_indirect_call_callee"),
>> > + get_identifier (
>> > + (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) ?
>> > + "__gcov_indirect_call_topn_callee" :
>> > + "__gcov_indirect_call_callee")),
>> > ptr_void);
>> > TREE_PUBLIC (ic_void_ptr_var) = 1;
>> > DECL_EXTERNAL (ic_void_ptr_var) = 1;
>> > @@ -131,7 +135,10 @@ init_ic_make_global_vars (void)
>> > {
>> > ic_gcov_type_ptr_var
>> > = build_decl (UNKNOWN_LOCATION, VAR_DECL,
>> > - get_identifier ("__gcov_indirect_call_counters"),
>> > + get_identifier (
>> > + (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) ?
>> > + "__gcov_indirect_call_topn_counters" :
>> > + "__gcov_indirect_call_counters")),
>> > gcov_type_ptr);
>> > TREE_PUBLIC (ic_gcov_type_ptr_var) = 1;
>> > DECL_EXTERNAL (ic_gcov_type_ptr_var) = 1;
>> > @@ -226,8 +233,10 @@ gimple_init_edge_profiler (void)
>> > ptr_void,
>> > NULL_TREE);
>> > tree_indirect_call_profiler_fn
>> > - = build_fn_decl ("__gcov_indirect_call_profiler_v2",
>> > - ic_profiler_fn_type);
>> > + = build_fn_decl ( (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) ?
>> > + "__gcov_indirect_call_topn_profiler":
>> > + "__gcov_indirect_call_profiler_v2"),
>> > + ic_profiler_fn_type);
>> > }
>> > TREE_NOTHROW (tree_indirect_call_profiler_fn) = 1;
>> > DECL_ATTRIBUTES (tree_indirect_call_profiler_fn)
>> > @@ -398,6 +407,12 @@ gimple_gen_ic_profiler (histogram_value value, uns
>> > gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>> > tree ref_ptr = tree_coverage_counter_addr (tag, base);
>> >
>> > + if ( (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) &&
>> > + tag == GCOV_COUNTER_V_INDIR) ||
>> > + (!PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) &&
>> > + tag == GCOV_COUNTER_ICALL_TOPNV))
>> > + return;
>> > +
>> > ref_ptr = force_gimple_operand_gsi (&gsi, ref_ptr,
>> > true, NULL_TREE, true, GSI_SAME_STMT);
>> >
>> > @@ -442,8 +457,7 @@ gimple_gen_ic_func_profiler (void)
>> > stmt1: __gcov_indirect_call_profiler_v2 (profile_id,
>> > ¤t_function_decl)
>> > */
>> > - gsi =
>> > - gsi_after_labels (split_edge (single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun))));
>> > + gsi = gsi_after_labels (split_edge (single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun))));
>> >
>> > cur_func = force_gimple_operand_gsi (&gsi,
>> > build_addr (current_function_decl,
>> > Index: gcc/value-prof.c
>> > ===================================================================
>> > --- gcc/value-prof.c (revision 215886)
>> > +++ gcc/value-prof.c (working copy)
>> > @@ -60,6 +60,7 @@ along with GCC; see the file COPYING3. If not see
>> > #include "builtins.h"
>> > #include "tree-nested.h"
>> > #include "hash-set.h"
>> > +#include "params.h"
>> >
>> > /* In this file value profile based optimizations are placed. Currently the
>> > following optimizations are implemented (for more detailed descriptions
>> > @@ -359,6 +360,22 @@ dump_histogram_value (FILE *dump_file, histogram_v
>> > }
>> > fprintf (dump_file, ".\n");
>> > break;
>> > + case HIST_TYPE_INDIR_CALL_TOPN:
>> > + fprintf (dump_file, "Indirect call topn ");
>> > + if (hist->hvalue.counters)
>> > + {
>> > + int i;
>> > +
>> > + fprintf (dump_file, "accu:%"PRId64, hist->hvalue.counters[0]);
>> > + for (i = 1; i < (GCOV_ICALL_TOPN_VAL << 2); i += 2)
>> > + {
>> > + fprintf (dump_file, " target:%"PRId64 " value:%"PRId64,
>> > + (int64_t) hist->hvalue.counters[i],
>> > + (int64_t) hist->hvalue.counters[i+1]);
>> > + }
>> > + }
>> > + fprintf (dump_file, ".\n");
>> > + break;
>> > case HIST_TYPE_MAX:
>> > gcc_unreachable ();
>> > }
>> > @@ -432,9 +449,14 @@ stream_in_histogram_value (struct lto_input_block
>> > break;
>> >
>> > case HIST_TYPE_IOR:
>> > - case HIST_TYPE_TIME_PROFILE:
>> > + case HIST_TYPE_TIME_PROFILE:
>> > ncounters = 1;
>> > break;
>> > +
>> > + case HIST_TYPE_INDIR_CALL_TOPN:
>> > + ncounters = (GCOV_ICALL_TOPN_VAL << 2) + 1;
>> > + break;
>> > +
>> > case HIST_TYPE_MAX:
>> > gcc_unreachable ();
>> > }
>> > @@ -1920,8 +1942,12 @@ gimple_indirect_call_to_profile (gimple stmt, hist
>> >
>> > values->reserve (3);
>> >
>> > - values->quick_push (gimple_alloc_histogram_value (cfun, HIST_TYPE_INDIR_CALL,
>> > - stmt, callee));
>> > + values->quick_push (gimple_alloc_histogram_value (
>> > + cfun,
>> > + PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) ?
>> > + HIST_TYPE_INDIR_CALL_TOPN :
>> > + HIST_TYPE_INDIR_CALL,
>> > + stmt, callee));
>> >
>> > return;
>> > }
>> > @@ -2011,9 +2037,9 @@ gimple_find_values_to_profile (histogram_values *v
>> > hist->n_counters = 3;
>> > break;
>> >
>> > - case HIST_TYPE_TIME_PROFILE:
>> > - hist->n_counters = 1;
>> > - break;
>> > + case HIST_TYPE_TIME_PROFILE:
>> > + hist->n_counters = 1;
>> > + break;
>> >
>> > case HIST_TYPE_AVERAGE:
>> > hist->n_counters = 2;
>> > @@ -2023,6 +2049,10 @@ gimple_find_values_to_profile (histogram_values *v
>> > hist->n_counters = 1;
>> > break;
>> >
>> > + case HIST_TYPE_INDIR_CALL_TOPN:
>> > + hist->n_counters = GCOV_ICALL_TOPN_NCOUNTS;
>> > + break;
>> > +
>> > default:
>> > gcc_unreachable ();
>> > }
>> > Index: gcc/value-prof.h
>> > ===================================================================
>> > --- gcc/value-prof.h (revision 215886)
>> > +++ gcc/value-prof.h (working copy)
>> > @@ -35,6 +35,8 @@ enum hist_type
>> > HIST_TYPE_AVERAGE, /* Compute average value (sum of all values). */
>> > HIST_TYPE_IOR, /* Used to compute expected alignment. */
>> > HIST_TYPE_TIME_PROFILE, /* Used for time profile */
>> > + HIST_TYPE_INDIR_CALL_TOPN, /* Tries to identify the top N most frequently
>> > + called functions in indirect call. */
>> > HIST_TYPE_MAX
>> > };
>> >
>> > Index: gcc/profile.c
>> > ===================================================================
>> > --- gcc/profile.c (revision 215886)
>> > +++ gcc/profile.c (working copy)
>> > @@ -183,6 +183,7 @@ instrument_values (histogram_values values)
>> > break;
>> >
>> > case HIST_TYPE_INDIR_CALL:
>> > + case HIST_TYPE_INDIR_CALL_TOPN:
>> > gimple_gen_ic_profiler (hist, t, 0);
>> > break;
>> >