Re: [PATCH] offline gcda profile processing tool

[Rong Xu <xur at google dot com>]

On Tue, Apr 15, 2014 at 2:38 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Rong, David, Dehao, Teresa
> I would like to have some rought idea of what we could merge this stage1. There is
> certainly a lot of interesting stuff on the google branch including AutoFDO, LIPO,
> the multivalue profile counters that may be used by the new devirtualization bits
> and more. I also think we should switch counts into floating point representation
> so Teresa's splitting patch works.
>
> Can we get plans to make this effective? My personal schedule is quite free until
> April 29 when I go to Czech Republic for wedding and I will be back in Calgary
> at 14th.
>
>> 2014-03-03  Rong Xu  <xur@google.com>
>>
>>       * gcc/gcov-io.c (gcov_read_string): Make this routine available
>>         to gcov-tool.
>>       (gcov_sync): Ditto.
>>       * gcc/Makefile.in: Build and install gcov-tool.
>>       * gcc/gcov-tool.c (unlink_gcda_file): Remove one gcda file.
>>       (unlink_profile_dir): Remove gcda files from the profile path.
>>       (profile_merge): Merge two profiles in directory.
>>       (print_merge_usage_message): Print merge usage.
>>       (merge_usage): Print merge usage and exit.
>>       (do_merge): Driver for profile merge sub-command.
>>       (profile_rewrite): Rewrite profile.
>>       (print_rewrite_usage_message): Print rewrite usage.
>>       (rewrite_usage): Print rewrite usage and exit.
>>       (do_rewrite): Driver for profile rewrite sub-command.
>>       (print_usage): Print gcov-info usage and exit.
>>       (print_version): Print gcov-info version.
>>       (process_args): Process arguments.
>>       (main): Main routine for gcov-tool.
>>       * libgcc/libgcov.h : Include the set of base-type headers for
>>         gcov-tool.
>>         (struct gcov_info): Make the functions field mutable in gcov-tool
>>         compilation.
>>       * libgcc/libgcov-merge.c (gcov_get_counter): New wrapper function
>>         to get the profile counter.
>>       (gcov_get_counter_target): New wrapper function to get the profile
>>         values that should not be scaled.
>>       (__gcov_merge_add): Replace gcov_read_counter() with the wrapper
>>         functions.
>>       (__gcov_merge_ior): Ditto.
>>       (__gcov_merge_time_profile): Ditto.
>>       (__gcov_merge_single): Ditto.
>>       (__gcov_merge_delta): Ditto.
>>       * libgcc/libgcov-util.c (void gcov_set_verbose): Set the verbose flag
>>         in the utility functions.
>>       (set_fn_ctrs): Utility function for reading gcda files to in-memory
>>         gcov_list object link lists.
>>       (tag_function): Ditto.
>>       (tag_blocks): Ditto.
>>       (tag_arcs): Ditto.
>>       (tag_lines): Ditto.
>>       (tag_counters): Ditto.
>>       (tag_summary): Ditto.
>>       (read_gcda_finalize): Ditto.
>>       (read_gcda_file): Ditto.
>>       (ftw_read_file): Ditto.
>>       (read_profile_dir_init): Ditto.
>>       (gcov_read_profile_dir): Ditto.
>>       (gcov_read_counter_mem): Ditto.
>>       (gcov_get_merge_weight): Ditto.
>>       (merge_wrapper): A wrapper function that calls merging handler.
>>       (gcov_merge): Merge two gcov_info objects with weights.
>>       (find_match_gcov_info): Find the matched gcov_info in the list.
>>       (gcov_profile_merge): Merge two gcov_info object lists.
>>       (__gcov_add_counter_op): Process edge profile counter values.
>>       (__gcov_ior_counter_op): Process IOR profile counter values.
>>       (__gcov_delta_counter_op): Process delta profile counter values.
>>       (__gcov_single_counter_op): Process single  profile counter values.
>>       (fp_scale): Callback function for float-point scaling.
>>       (int_scale): Callback function for integer fraction scaling.
>>       (gcov_profile_scale): Scaling profile counters.
>>       (gcov_profile_normalize): Normalize profile counters.
>>
>> Index: gcc/gcov-io.c
>> ===================================================================
>> --- gcc/gcov-io.c     (revision 208237)
>> +++ gcc/gcov-io.c     (working copy)
>> @@ -564,7 +564,7 @@ gcov_read_counter (void)
>>     buffer, or NULL on empty string. You must copy the string before
>>     calling another gcov function.  */
>>
>> -#if !IN_LIBGCOV
>> +#if !IN_LIBGCOV || defined (IN_GCOV_TOOL)
>>  GCOV_LINKAGE const char *
>>  gcov_read_string (void)
>>  {
>> @@ -641,7 +641,7 @@ gcov_read_summary (struct gcov_summary *summary)
>>      }
>>  }
>>
>> -#if !IN_LIBGCOV
>> +#if !IN_LIBGCOV || defined (IN_GCOV_TOOL)
>>  /* Reset to a known position.  BASE should have been obtained from
>>     gcov_position, LENGTH should be a record length.  */
>
> I am slightly confused here, IN_LIBGCOV IMO means that the gcov-io is going
> to be linked into the gcov runtime as opposed to gcc, gcov, gcov-dump or
> gcov-tool.  Why we define IN_LIBGCOV && IN_GCOV_TOOL?

GCOT_TOOL needs to use this function to read the string in gcda file
to memory to construct gcov_info objects.
As you noticed, gcov runtime does not need this interface. But
gcov-tool links with gcov runtime and it also uses the function.
We could make it available in gcov_runtime, but that will slightly
increase the memory footprint.

>> Index: gcc/gcov-tool.c
>> ===================================================================
>> --- gcc/gcov-tool.c   (revision 0)
>> +++ gcc/gcov-tool.c   (revision 0)
>> @@ -0,0 +1,465 @@
>> +/* Gcc offline profile processing tool support. */
>> +/* Compile this one with gcc.  */
>> +/* Copyright (C) 2014 Free Software Foundation, Inc.
>> +   Contributed by Rong Xu <xur@google.com>.
>> +
>> +This file is part of GCC.
>> +
>> +GCC is free software; you can redistribute it and/or modify it under
>> +the terms of the GNU General Public License as published by the Free
>> +Software Foundation; either version 3, or (at your option) any later
>> +version.
>> +
>> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
>> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> +for more details.
>> +
>> +Under Section 7 of GPL version 3, you are granted additional
>> +permissions described in the GCC Runtime Library Exception, version
>> +3.1, as published by the Free Software Foundation.
>> +
>> +You should have received a copy of the GNU General Public License and
>> +a copy of the GCC Runtime Library Exception along with this program;
>> +see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>> +<http://www.gnu.org/licenses/>.  */
>> +
>> +#include "config.h"
>> +#include "system.h"
>> +#include "coretypes.h"
>> +#include "tm.h"
>> +#include "intl.h"
>> +#include "diagnostic.h"
>> +#include "version.h"
>> +#include "gcov-io.h"
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <ftw.h>
>> +#include <getopt.h>
>> +
>
> I glanced only briefly over the sources, they look resonable. I suppose we will
> gain more functionality soon?

The planned new functions for trunk version is: (1) overlap score b/w
two profiles (2) better dumping (top hot objects/function/counters)
and statistics.
Once this basic version is in, we can start to add the new functionality.

>
>> +#ifndef IN_GCOV_TOOL
>> +/* About the target.  */
>> +
>>  #include "tconfig.h"
>>  #include "tsystem.h"
>>  #include "coretypes.h"
>> @@ -79,6 +82,25 @@ typedef unsigned gcov_type_unsigned __attribute__
>>  #define GCOV_LOCKED 0
>>  #endif
>>
>> +#else /* IN_GCOV_TOOL */
>> +/* About the host.  */
>> +
>> +#include "config.h"
>> +#include "system.h"
>> +#include "coretypes.h"
>> +#include "tm.h"
>> +
>> +typedef unsigned gcov_unsigned_t;
>> +typedef unsigned gcov_position_t;
>> +/* gcov_type is typedef'd elsewhere for the compiler */
>> +#if defined (HOST_HAS_F_SETLKW)
>> +#define GCOV_LOCKED 1
>> +#else
>> +#define GCOV_LOCKED 0
>> +#endif
>> +
>> +#endif /* !IN_GCOV_TOOL */
>
> I see, so the purepose of IN_GCOV_TOOL is that you link special purpose libgcov
> into gcov-tool.
> We at least need a documentation for this.

OK. Will document in the updated patch.

>> Index: libgcc/libgcov-merge.c
>> ===================================================================
>> --- libgcc/libgcov-merge.c    (revision 208237)
>> +++ libgcc/libgcov-merge.c    (working copy)
>> @@ -45,6 +45,26 @@ void __gcov_merge_delta (gcov_type *counters  __at
>>
>>  #else
>>
>> +static inline gcov_type
>> +gcov_get_counter (void)
>> +{
>> +#ifndef IN_GCOV_TOOL
>> +  return gcov_read_counter ();
>> +#else
>> +  return gcov_read_counter_mem () * gcov_get_merge_weight ();
>> +#endif
>> +}
>> +
>> +static inline gcov_type
>> +gcov_get_counter_target (void)
>> +{
>> +#ifndef IN_GCOV_TOOL
>> +  return gcov_read_counter ();
>> +#else
>> +  return gcov_read_counter_mem ();
>> +#endif
>> +}
>
> These functions needs documentation.
> We now have way to separate runtime from basic IO and yet we have those rather
> ugly ifdefs.  Can't we put these into an separate include rather than spreading
> ifdefs around the code?

OK. I'll try to refactor this in the new patch.

>
>> Index: libgcc/libgcov-util.c
>> ===================================================================
>> --- libgcc/libgcov-util.c     (revision 0)
>> +++ libgcc/libgcov-util.c     (revision 0)
>> @@ -0,0 +1,855 @@
>> +/* Utility functions for reading gcda files into in-memory
>> +   gcov_info structures and offline profile processing. */
>> +/* Copyright (C) 2014 Free Software Foundation, Inc.
>> +   Contributed by Rong Xu <xur@google.com>.
>> +
>> +This file is part of GCC.
>> +
>> +GCC is free software; you can redistribute it and/or modify it under
>> +the terms of the GNU General Public License as published by the Free
>> +Software Foundation; either version 3, or (at your option) any later
>> +version.
>> +
>> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
>> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> +for more details.
>> +
>> +Under Section 7 of GPL version 3, you are granted additional
>> +permissions described in the GCC Runtime Library Exception, version
>> +3.1, as published by the Free Software Foundation.
>> +
>> +You should have received a copy of the GNU General Public License and
>> +a copy of the GCC Runtime Library Exception along with this program;
>> +see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>> +<http://www.gnu.org/licenses/>.  */
>> +
>> +
>> +#define IN_GCOV_TOOL 1
>> +#define L_gcov 1
>> +#define L_gcov_merge_add 1
>> +#define L_gcov_merge_single 1
>> +#define L_gcov_merge_delta 1
>> +#define L_gcov_merge_ior 1
>> +#define L_gcov_merge_time_profile 1
>> +
>> +#include "libgcov.h"
>> +#include "intl.h"
>> +#include "diagnostic.h"
>> +#include "version.h"
>> +#include "demangle.h"
>> +
>> +extern gcov_type gcov_read_counter_mem();
>> +extern unsigned gcov_get_merge_weight();
>> +
>> +/* We need the dumping and merge part of code in libgcov.  */
>> +#include "libgcov-driver.c"
>> +#include "libgcov-merge.c"
>
> This is rather ugly. I would preffer Makefile building libgcov-driver/libgcov-merge
> separately for gcov tool. Would that be too hard to arrange?
>
> How libgcov-util is linked into the tool?

libgcov-util.o is built in gcc/ directory, rather in libgcc.
It's directly linked to gcov-tool.
So libgcov-util.o is built for HOST, not TARGET.
With makefile changes, we can built HOST version of libgcov-driver.o
and libgcov-merge.o.
I also need to make some static functions/variables public.

I did the way in this patch because it incurs least change in existing files.
But, yes, your preferred is doable.

>> +
>> +/* Merge functions for counters.  */
>> +static gcov_merge_fn ctr_merge_functions[GCOV_COUNTERS] = {
>> +    __gcov_merge_add,
>> +    __gcov_merge_add,
>> +    __gcov_merge_add,
>> +    __gcov_merge_single,
>> +    __gcov_merge_delta,
>> +    __gcov_merge_single,
>> +    __gcov_merge_add,
>> +    __gcov_merge_ior,
>> +    __gcov_merge_time_profile,
>> +};
>
> We are gathering more and more places info about profilers is spread
> and this seems to duplicate GCOV_MERGE_FUNCTIONS except for the "..".
> Can't we go with gcov-counters.def file that summarizes it at one place?

OK. I'll try to re-factor this.

>> +
>> +/* Set the ctrs field in gcov_fn_info object FN_INFO.  */
>> +
>> +static void
>> +set_fn_ctrs (struct gcov_fn_info *fn_info)
>> +{
>> +  int j = 0, i;
>> +
>> +  for (i = 0; i < GCOV_COUNTERS; i++)
>> +    {
>> +      if (k_ctrs_mask[i] == 0)
>> +        continue;
>> +      fn_info->ctrs[j].num = k_ctrs[i].num;
>> +      fn_info->ctrs[j].values = k_ctrs[i].values;
>> +      j++;
>> +    }
>> +  if (k_ctrs_types == 0)
>> +    k_ctrs_types = j;
>> +  else
>> +    gcc_assert (j == k_ctrs_types);
>> +}
>> +
>> +typedef struct tag_format
>> +{
>> +    unsigned tag;
>> +    char const *name;
>> +    void (*proc) (unsigned, unsigned);
>> +} tag_format_t;
>> +
>> +static const tag_format_t tag_table[] =
>
> This needs documentation.

Will do in the new patch.

>> +{
>> +  {0, "NOP", NULL},
>> +  {0, "UNKNOWN", NULL},
>> +  {0, "COUNTERS", tag_counters},
>> +  {GCOV_TAG_FUNCTION, "FUNCTION", tag_function},
>> +  {GCOV_TAG_BLOCKS, "BLOCKS", tag_blocks},
>> +  {GCOV_TAG_ARCS, "ARCS", tag_arcs},
>> +  {GCOV_TAG_LINES, "LINES", tag_lines},
>> +  {GCOV_TAG_OBJECT_SUMMARY, "OBJECT_SUMMARY", tag_summary},
>> +  {GCOV_TAG_PROGRAM_SUMMARY, "PROGRAM_SUMMARY", tag_summary},
>> +  {0, NULL, NULL}
>> +};
>> +/* Handler for reading block tag.  */
>> +
>> +static void
>> +tag_blocks (unsigned tag ATTRIBUTE_UNUSED, unsigned length ATTRIBUTE_UNUSED)
>> +{
>> +  gcc_assert (0);
>> +}
>> +
>> +/* Handler for reading flow arc tag.  */
>> +
>> +static void
>> +tag_arcs (unsigned tag ATTRIBUTE_UNUSED, unsigned length ATTRIBUTE_UNUSED)
>> +{
>> +  gcc_assert (0);
>> +}
>> +
>> +/* Handler for reading line tag.  */
>> +
>> +static void
>> +tag_lines (unsigned tag ATTRIBUTE_UNUSED, unsigned length ATTRIBUTE_UNUSED)
>> +{
>> +  gcc_assert (0);
>> +}
> gcc_unreachable? Perhaps with a comment why those are not read in gcda?

gcov-tool currently does not handle gcno file.
Will add comments here.

>> +
>> +/* Performaing FN upon delta counters.  */
>> +
>> +static void
>> +__gcov_delta_counter_op (gcov_type *counters, unsigned n_counters,
>> +                         counter_op_fn fn, void *data1, void *data2)
>> +{
>> +  unsigned i, n_measures;
>> +
>> +  gcc_assert (!(n_counters % 4));
>> +  n_measures = n_counters / 4;
>> +  for (i = 0; i < n_measures; i++, counters += 4)
>> +    {
>> +      counters[2] = fn (counters[2], data1, data2);
>> +      counters[3] = fn (counters[3], data1, data2);
>> +    }
>> +}
>> +
>> +/* Performing FN upon single counters.  */
>> +
>> +static void
>> +__gcov_single_counter_op (gcov_type *counters, unsigned n_counters,
>> +                          counter_op_fn fn, void *data1, void *data2)
>> +{
>> +  unsigned i, n_measures;
>> +
>> +  gcc_assert (!(n_counters % 3));
>> +  n_measures = n_counters / 3;
>> +  for (i = 0; i < n_measures; i++, counters += 3)
>> +    {
>> +      counters[1] = fn (counters[1], data1, data2);
>> +      counters[2] = fn (counters[2], data1, data2);
>> +    }
>
> Won't this get wrong answer when counters[0] is not the same?
> I would expected the merging code to compare the counters first. Similarly for delta counter.

These *_op functions are for scaling only. So there is only one
profile involved (thus there is no comparison).
The merge handles are in libgcov-merge.c which have the code to handle
mismatched profile targets.

>> +
>> +/* Scaling the counter value V by multiplying *(float*) DATA1.  */
>> +
>> +static gcov_type
>> +fp_scale (gcov_type v, void *data1, void *data2 ATTRIBUTE_UNUSED)
>> +{
>> +  float f = *(float *) data1;
>> +  return (gcov_type) (v * f);
>> +}
>> +
>> +/* Scaling the counter value V by multiplying DATA2/DATA1.  */
>> +
>> +static gcov_type
>> +int_scale (gcov_type v, void *data1, void *data2)
>> +{
>> +  int n = *(int *) data1;
>> +  int d = *(int *) data2;
>> +  return (gcov_type) ((v / d) * n);
>> +}
>
> Adding correct rounding may actually make difference for Martin's startup time work.

Do you mean to use something like in RDIV macro?

>
> The patch looks resonable in general.  I am just concerned about the interfaces within libgcov.
> In a way it seems to me that there ought to be ways to make this cleaner without that many of
> ifdefs.
> Please send updated patch with the changes above and if you have any ideas for cleanups
> of the interfaces, I would definitely welcome them.

Thanks for the review and suggestion. I'll send an updated patch.

>
> Honza