Re: [PATCH] Time profiler - phase 1

[Martin LiÅka <marxin dot liska at gmail dot com>]

On 11 November 2013 15:57, Jan Hubicka <hubicka@ucw.cz> wrote:
>> +2013-10-29  Martin Liska  <marxin.liska@gmail.com>
>> +                                             Jan Hubicka  <jh@suse.cz>
>> +
>> +     * cgraph.c (dump_cgraph_node): Profile dump added.
>> +     * cgraph.h (struct cgraph_node): New time profile variable added.
>> +     * cgraphclones.c (cgraph_clone_node): Time profile is cloned.
>> +     * gcov-io.h (gcov_type): New profiler type introduced.
>> +     * ipa-profile.c (lto_output_node): Streaming for time profile added.
>> +     (input_node): Time profiler is read from LTO stream.
>> +     * predict.c (maybe_hot_count_p): Hot prediction changed.
>> +     * profile.c (instrument_values): New case for time profiler added.
>> +     (compute_value_histograms): Read of time profile.
>> +     * tree-pretty-print.c (dump_function_header): Time profiler is dumped.
>> +     * tree-profile.c (init_ic_make_global_vars): Time profiler function added.
>> +     (gimple_init_edge_profiler): TP function instrumentation.
>> +     (gimple_gen_time_profiler): New.
>> +     * value-prof.c (gimple_add_histogram_value): Support for time profiler
>> +     added.
>> +     (dump_histogram_value): TP type added to dumps.
>> +     (visit_hist): More sensitive check that takes TP into account.
>> +     (gimple_find_values_to_profile): TP instrumentation.
>> +     * value-prof.h (hist_type): New histogram type added.
>> +     (struct histogram_value_t): Pointer to struct function added.
>> +     * libgcc/Makefile.in: New GCOV merge function for TP added.
>> +     * libgcov.c: function_counter variable introduced.
>> +     (_gcov_merge_time_profile): New.
>> +     (_gcov_time_profiler): New.
>> +
>>  2013-11-05  Steven Bosscher  <steven@gcc.gnu.org>
>>
>>
>> diff --git a/gcc/ipa-profile.c b/gcc/ipa-profile.c
>> index 1260069..eff4b51 100644
>> --- a/gcc/ipa-profile.c
>> +++ b/gcc/ipa-profile.c
>> @@ -465,6 +465,7 @@ ipa_propagate_frequency (struct cgraph_node *node)
>>    if (d.maybe_unlikely_executed)
>>      {
>>        node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
>> +      node->tp_first_run = 0;
>>        if (dump_file)
>>       fprintf (dump_file, "Node %s promoted to unlikely executed.\n",
>>                cgraph_node_name (node));
>
> Why do you put tp_first_run to 0?  The function may be unlikely but it still may have
> average possition in the program execution. I.e. when it is executed once per many
> invocations during the train run)

That makes no sense, I've removed this assignment.

>> @@ -895,9 +907,22 @@ compute_value_histograms (histogram_values values, unsigned cfg_checksum,
>>        hist->hvalue.counters =  XNEWVEC (gcov_type, hist->n_counters);
>>        for (j = 0; j < hist->n_counters; j++)
>>          if (aact_count)
>> -       hist->hvalue.counters[j] = aact_count[j];
>> -     else
>> -       hist->hvalue.counters[j] = 0;
>> +          hist->hvalue.counters[j] = aact_count[j];
>> +        else
>> +          hist->hvalue.counters[j] = 0;
>> +
>> +      /* Time profiler counter is not related to any statement,
>> +       * so that we have to read the counter and set the value to
>> +       * the corresponding call graph node.  */
>
> Reformat comment to GNU style.

Yes.

>> +2013-10-28  Martin Liska     <marxin.liska@gmail.com>
>> +
>> +     * gcc.dg/time-profiler-1.c: New test.
>> +     * gcc.dg/time-profiler-2.c: Ditto.
>> +
> It is custom to put all changelogs to the beggining of your patch (just nitpicking ;))
>> @@ -222,6 +225,18 @@ gimple_init_edge_profiler (void)
>>       = tree_cons (get_identifier ("leaf"), NULL,
>>                    DECL_ATTRIBUTES (tree_indirect_call_profiler_fn));
>>
>> +      /* void (*) (gcov_type *, gcov_type, void *)  */
>> +      time_profiler_fn_type
>> +            = build_function_type_list (void_type_node,
>> +                                       gcov_type_ptr, NULL_TREE);
>> +      tree_time_profiler_fn
>> +           = build_fn_decl ("__gcov_time_profiler",
>> +                                  time_profiler_fn_type);
>> +      TREE_NOTHROW (tree_time_profiler_fn) = 1;
>> +      DECL_ATTRIBUTES (tree_time_profiler_fn)
>> +     = tree_cons (get_identifier ("leaf"), NULL,
>> +                  DECL_ATTRIBUTES (tree_time_profiler_fn));
>
> We should udpate this code to use set_call_expr_flags but by incremental patch.

OK, I will include this change to the phase 2.

> The patch is OK with changes above.  Do you have write permissin to commit it?
> If not, just send updated version and I will commit it + follow the write access
> instructions on gcc.gnu.org homepage.

Yes, I do have commit right. I will bootstrap the patch, test Inkscape
instrumentation and commit it.

Martin

> Honza