Re: [google] Instrumented sampling FDO interface cleanup (issue6210058)

[Xinliang David Li <davidxl at google dot com>]

Looks good.

thanks,

David

On Mon, May 14, 2012 at 7:18 PM, Teresa Johnson <tejohnson@google.com> wrote:
> Two cleanup items for the sampling instrumentation interfaces. First,
> rename variables from *rate* to *period*, since what is being specified
> is a sampling period (time between recorded samples) not a rate.
> Second, add flag __gcov_has_sampling to indicate when a file was
> compiled with -fprofile-generate-sampling, and fuction
> __gcov_sampling_enabled to return the state of this flag. The flag
> is checked when a call is made to set the sampling period.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for google branches?
>
> Thanks,
> Teresa
>
> 2012-05-14 ? Teresa Johnson ?<tejohnson@google.com>
>
> ? ? ? ?* libgcc/libgcov.c (__gcov_sampling_period): Rename variable from
> ? ? ? ?"*rate*" to "*period*".
> ? ? ? ?(gcov_sampling_period_initialized): Ditto.
> ? ? ? ?(__gcov_set_sampling_period): Rename from __gcov_set_sampling_rate.
> ? ? ? ?Check __gcov_has_sampling.
> ? ? ? ?(__gcov_has_sampling): New variable.
> ? ? ? ?(__gcov_sampling_enabled): New function.
> ? ? ? ?(__gcov_init): Rename variables from "*rate*" to "*period*".
> ? ? ? ?* gcc/doc/invoke.texi (profile-generate-sampling-period): Rename
> ? ? ? ?from profile-generate-sampling-rate.
> ? ? ? ?* gcc/tree-profile.c (gcov_sampling_period_decl): Rename from
> ? ? ? ?gcov_sampling_rate_decl.
> ? ? ? ?(gcov_has_sampling_decl): New variable.
> ? ? ? ?(insert_if_then, add_sampling_wrapper): Rename variables from
> ? ? ? ?"*rate*" to "*period*".
> ? ? ? ?(gimple_init_instrumentation_sampling): Ditto, and add handling
> ? ? ? ?for gcov_has_sampling_decl.
> ? ? ? ?* gcc/params.def (profile-generate-sampling-period): Rename
> ? ? ? ?from profile-generate-sampling-rate.
>
> Index: libgcc/libgcov.c
> ===================================================================
> --- libgcc/libgcov.c ? ?(revision 187470)
> +++ libgcc/libgcov.c ? ?(working copy)
> @@ -141,18 +141,26 @@ extern char * __gcov_pmu_profile_filename;
> ?extern char * __gcov_pmu_profile_options;
> ?extern gcov_unsigned_t __gcov_pmu_top_n_address;
>
> -/* Sampling rate. ?*/
> -extern gcov_unsigned_t __gcov_sampling_rate;
> -static int gcov_sampling_rate_initialized = 0;
> -void __gcov_set_sampling_rate (unsigned int rate);
> +/* Sampling period. ?*/
> +extern gcov_unsigned_t __gcov_sampling_period;
> +extern gcov_unsigned_t __gcov_has_sampling;
> +static int gcov_sampling_period_initialized = 0;
> +void __gcov_set_sampling_period (unsigned int period);
> +unsigned int __gcov_sampling_enabled ();
>
> -/* Set sampling rate to RATE. ?*/
> +/* Set sampling period to PERIOD. ?*/
>
> -void __gcov_set_sampling_rate (unsigned int rate)
> +void __gcov_set_sampling_period (unsigned int period)
> ?{
> - ?__gcov_sampling_rate = rate;
> + ?gcc_assert (__gcov_has_sampling);
> + ?__gcov_sampling_period = period;
> ?}
>
> +unsigned int __gcov_sampling_enabled ()
> +{
> + ?return __gcov_has_sampling;
> +}
> +
> ?/* Per thread sample counter. ?*/
> ?THREAD_PREFIX gcov_unsigned_t __gcov_sample_counter = 0;
>
> @@ -659,16 +667,16 @@ gcov_clear (void)
> ?void
> ?__gcov_init (struct gcov_info *info)
> ?{
> - ?if (!gcov_sampling_rate_initialized)
> + ?if (!gcov_sampling_period_initialized)
> ? ? {
> - ? ? ?const char* env_value_str = getenv ("GCOV_SAMPLING_RATE");
> + ? ? ?const char* env_value_str = getenv ("GCOV_SAMPLING_PERIOD");
> ? ? ? if (env_value_str)
> ? ? ? ? {
> ? ? ? ? ? int env_value_int = atoi(env_value_str);
> ? ? ? ? ? if (env_value_int >= 1)
> - ? ? ? ? ? ?__gcov_sampling_rate = env_value_int;
> + ? ? ? ? ? ?__gcov_sampling_period = env_value_int;
> ? ? ? ? }
> - ? ? ?gcov_sampling_rate_initialized = 1;
> + ? ? ?gcov_sampling_period_initialized = 1;
> ? ? }
>
> ? if (!info->version || !info->n_functions)
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi (revision 187470)
> +++ gcc/doc/invoke.texi (working copy)
> @@ -8335,12 +8335,12 @@ the profile feedback data files. See @option{-fpro
> ?@opindex -fprofile-generate-sampling
>
> ?Enable sampling for instrumented binaries. ?Instead of recording every event,
> -record only every N-th event, where N (the sampling rate) can be set either
> +record only every N-th event, where N (the sampling period) can be set either
> ?at compile time using
> -@option{--param profile-generate-sampling-rate=@var{value}}, or
> -at execution start time through environment variable @samp{GCOV_SAMPLING_RATE}.
> +@option{--param profile-generate-sampling-period=@var{value}}, or at
> +execution start time through environment variable @samp{GCOV_SAMPLING_PERIOD}.
>
> -At this time sampling applies only to branch counters. ?A sampling rate of 100
> +At this time sampling applies only to branch counters. ?A sampling period of 100
> ?decreases instrumentated binary slowdown from up to 20x for heavily threaded
> ?applications down to around 2x. ?@option{-fprofile-correction} is always
> ?needed with sampling.
> Index: gcc/tree-profile.c
> ===================================================================
> --- gcc/tree-profile.c ?(revision 187470)
> +++ gcc/tree-profile.c ?(working copy)
> @@ -165,9 +165,12 @@ static struct pointer_set_t *instrumentation_to_be
> ?/* extern __thread gcov_unsigned_t __gcov_sample_counter ?*/
> ?static tree gcov_sample_counter_decl = NULL_TREE;
>
> -/* extern gcov_unsigned_t __gcov_sampling_rate ?*/
> -static tree gcov_sampling_rate_decl = NULL_TREE;
> +/* extern gcov_unsigned_t __gcov_sampling_period ?*/
> +static tree gcov_sampling_period_decl = NULL_TREE;
>
> +/* extern gcov_unsigned_t __gcov_has_sampling ?*/
> +static tree gcov_has_sampling_decl = NULL_TREE;
> +
> ?/* forward declaration. ?*/
> ?void gimple_init_instrumentation_sampling (void);
>
> @@ -200,7 +203,7 @@ insert_if_then (gimple stmt_start, gimple stmt_end
> ? ?Into:
>
> ? ?__gcov_sample_counter++;
> - ? if (__gcov_sample_counter >= __gcov_sampling_rate)
> + ? if (__gcov_sample_counter >= __gcov_sampling_period)
> ? ? ?{
> ? ? ? ?__gcov_sample_counter = 0;
> ? ? ? ?ORIGINAL CODE
> @@ -214,7 +217,7 @@ add_sampling_wrapper (gimple stmt_start, gimple st
> ?{
> ? tree zero, one, tmp_var, tmp1, tmp2, tmp3;
> ? gimple stmt_inc_counter1, stmt_inc_counter2, stmt_inc_counter3;
> - ?gimple stmt_reset_counter, stmt_assign_rate, stmt_if;
> + ?gimple stmt_reset_counter, stmt_assign_period, stmt_if;
> ? gimple_stmt_iterator gsi;
>
> ? tmp_var = create_tmp_reg (get_gcov_unsigned_t (), "PROF_sample");
> @@ -230,7 +233,7 @@ add_sampling_wrapper (gimple stmt_start, gimple st
> ? zero = build_int_cst (get_gcov_unsigned_t (), 0);
> ? stmt_reset_counter = gimple_build_assign (gcov_sample_counter_decl, zero);
> ? tmp3 = make_ssa_name (tmp_var, NULL);
> - ?stmt_assign_rate = gimple_build_assign (tmp3, gcov_sampling_rate_decl);
> + ?stmt_assign_period = gimple_build_assign (tmp3, gcov_sampling_period_decl);
> ? stmt_if = gimple_build_cond (GE_EXPR, tmp2, tmp3, NULL_TREE, NULL_TREE);
>
> ? /* Insert them for now in the original basic block. ?*/
> @@ -238,7 +241,7 @@ add_sampling_wrapper (gimple stmt_start, gimple st
> ? gsi_insert_before (&gsi, stmt_inc_counter1, GSI_SAME_STMT);
> ? gsi_insert_before (&gsi, stmt_inc_counter2, GSI_SAME_STMT);
> ? gsi_insert_before (&gsi, stmt_inc_counter3, GSI_SAME_STMT);
> - ?gsi_insert_before (&gsi, stmt_assign_rate, GSI_SAME_STMT);
> + ?gsi_insert_before (&gsi, stmt_assign_period, GSI_SAME_STMT);
> ? gsi_insert_before (&gsi, stmt_reset_counter, GSI_SAME_STMT);
>
> ? /* Insert IF block. ?*/
> @@ -293,27 +296,49 @@ add_sampling_to_edge_counters (void)
> ?void
> ?gimple_init_instrumentation_sampling (void)
> ?{
> - ?if (!gcov_sampling_rate_decl)
> + ?if (!gcov_sampling_period_decl)
> ? ? {
> - ? ? ?/* Define __gcov_sampling_rate regardless of -fprofile-generate-sampling.
> - ? ? ? ? Otherwise the extern reference to it from libgcov becomes unmatched.
> + ? ? ?/* Define __gcov_sampling_period regardless of
> + ? ? ? ? -fprofile-generate-sampling. Otherwise the extern reference to
> + ? ? ? ? it from libgcov becomes unmatched.
> ? ? ? */
> - ? ? ?gcov_sampling_rate_decl = build_decl (
> + ? ? ?gcov_sampling_period_decl = build_decl (
> ? ? ? ? ? UNKNOWN_LOCATION,
> ? ? ? ? ? VAR_DECL,
> - ? ? ? ? ?get_identifier ("__gcov_sampling_rate"),
> + ? ? ? ? ?get_identifier ("__gcov_sampling_period"),
> ? ? ? ? ? get_gcov_unsigned_t ());
> - ? ? ?TREE_PUBLIC (gcov_sampling_rate_decl) = 1;
> - ? ? ?DECL_ARTIFICIAL (gcov_sampling_rate_decl) = 1;
> - ? ? ?DECL_COMDAT_GROUP (gcov_sampling_rate_decl)
> - ? ? ? ? ?= DECL_ASSEMBLER_NAME (gcov_sampling_rate_decl);
> - ? ? ?TREE_STATIC (gcov_sampling_rate_decl) = 1;
> - ? ? ?DECL_INITIAL (gcov_sampling_rate_decl) = build_int_cst (
> + ? ? ?TREE_PUBLIC (gcov_sampling_period_decl) = 1;
> + ? ? ?DECL_ARTIFICIAL (gcov_sampling_period_decl) = 1;
> + ? ? ?DECL_COMDAT_GROUP (gcov_sampling_period_decl)
> + ? ? ? ? ?= DECL_ASSEMBLER_NAME (gcov_sampling_period_decl);
> + ? ? ?TREE_STATIC (gcov_sampling_period_decl) = 1;
> + ? ? ?DECL_INITIAL (gcov_sampling_period_decl) = build_int_cst (
> ? ? ? ? ? get_gcov_unsigned_t (),
> - ? ? ? ? ?PARAM_VALUE (PARAM_PROFILE_GENERATE_SAMPLING_RATE));
> - ? ? ?varpool_finalize_decl (gcov_sampling_rate_decl);
> + ? ? ? ? ?PARAM_VALUE (PARAM_PROFILE_GENERATE_SAMPLING_PERIOD));
> + ? ? ?varpool_finalize_decl (gcov_sampling_period_decl);
> ? ? }
>
> + ?if (!gcov_has_sampling_decl)
> + ? ?{
> + ? ? ?/* Initialize __gcov_has_sampling to 1 if -fprofile-generate-sampling
> + ? ? ? ? specified, 0 otherwise. Used by libgcov to determine whether
> + ? ? ? ? a request to set the sampling period makes sense. ?*/
> + ? ? ?gcov_has_sampling_decl = build_decl (
> + ? ? ? ? ?UNKNOWN_LOCATION,
> + ? ? ? ? ?VAR_DECL,
> + ? ? ? ? ?get_identifier ("__gcov_has_sampling"),
> + ? ? ? ? ?get_gcov_unsigned_t ());
> + ? ? ?TREE_PUBLIC (gcov_has_sampling_decl) = 1;
> + ? ? ?DECL_ARTIFICIAL (gcov_has_sampling_decl) = 1;
> + ? ? ?DECL_COMDAT_GROUP (gcov_has_sampling_decl)
> + ? ? ? ? ?= DECL_ASSEMBLER_NAME (gcov_has_sampling_decl);
> + ? ? ?TREE_STATIC (gcov_has_sampling_decl) = 1;
> + ? ? ?DECL_INITIAL (gcov_has_sampling_decl) = build_int_cst (
> + ? ? ? ? ?get_gcov_unsigned_t (),
> + ? ? ? ? ?flag_profile_generate_sampling ? 1 : 0);
> + ? ? ?varpool_finalize_decl (gcov_has_sampling_decl);
> + ? ?}
> +
> ? if (flag_profile_generate_sampling && !instrumentation_to_be_sampled)
> ? ? {
> ? ? ? instrumentation_to_be_sampled = pointer_set_create ();
> Index: gcc/params.def
> ===================================================================
> --- gcc/params.def ? ? ?(revision 187470)
> +++ gcc/params.def ? ? ?(working copy)
> @@ -919,8 +919,8 @@ DEFPARAM (PARAM_MAX_LIPO_MEMORY,
> ? ? ? ? ?"don't import aux files if memory consumption exceeds this value",
> ? ? ? ? ?2400000, 0, 0)
>
> -DEFPARAM (PARAM_PROFILE_GENERATE_SAMPLING_RATE,
> - ? ? ? ? "profile-generate-sampling-rate",
> +DEFPARAM (PARAM_PROFILE_GENERATE_SAMPLING_PERIOD,
> + ? ? ? ? "profile-generate-sampling-period",
> ? ? ? ? ?"sampling rate with -fprofile-generate-sampling",
> ? ? ? ? ?100, 0, 2000000000)
>
>
> --
> This patch is available for review at http://codereview.appspot.com/6210058