On Tue, Apr 15, 2014 at 2:38 PM, Jan Hubicka <hubi...@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 <x...@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 <x...@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 <x...@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
