[PATCH] offline gcda profile processing tool
Jan Hubicka
hubicka@ucw.cz
Tue Apr 15 22:05:00 GMT 2014
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?
> 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?
> +#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.
> 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?
> 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?
> +
> +/* 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?
> +
> +/* 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.
> +{
> + {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?
> +
> +/* 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.
> +
> +/* 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.
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.
Honza
More information about the Gcc-patches
mailing list