> Hi,
>
> I'm planning to port the AutoFDO patch upstream. Attached is the
> prepared patch. You can also find the patch in
> http://codereview.appspot.com/99010043
>
> I've tested the patch with SPECCPU2006. For the CINT2006 benchmarks,
> the speedup comparison between O2, FDO and AutoFDO is as follows:
>
> Reference: o2
> (1): auto_fdo
> (2): fdo
>
> Benchmark Base:Reference (1) (2)
> -----------------------------------------------------------------
> spec/2006/int/C++/471.omnetpp 23.18 +3.11% +5.09%
> spec/2006/int/C++/473.astar 21.15 +6.79% +9.80%
> spec/2006/int/C++/483.xalancbmk 36.68 +11.56% +14.47%
> spec/2006/int/C/400.perlbench 34.57 +6.59% +18.56%
> spec/2006/int/C/401.bzip2 23.17 +0.95% +2.49%
> spec/2006/int/C/403.gcc 32.33 +8.27% +9.76%
> spec/2006/int/C/429.mcf 42.13 +4.72% +5.23%
> spec/2006/int/C/445.gobmk 26.53 -1.39% +0.05%
> spec/2006/int/C/456.hmmer 23.72 +7.12% +7.87%
> spec/2006/int/C/458.sjeng 26.17 +4.65% +6.04%
> spec/2006/int/C/462.libquantum 57.23 +4.04% +1.42%
> spec/2006/int/C/464.h264ref 46.3 +1.07% +8.97%
>
> geometric mean +4.73% +7.36%
The results are very nice indeed. So basically it adds another 50% to
static estimation, that is not bad.
The patch is long and missing a changelog, I will add my comments and
I think it would be great to break it up where possible - for some
infrastructure preparation patches and the actual reading infrastructure.
> Index: gcc/cfghooks.c
> ===================================================================
> --- gcc/cfghooks.c (revision 210180)
> +++ gcc/cfghooks.c (working copy)
> @@ -833,6 +833,9 @@ make_forwarder_block (basic_block bb, bool (*redir
>
> fallthru = split_block_after_labels (bb);
> dummy = fallthru->src;
> + dummy->count = 0;
> + dummy->frequency = 0;
> + fallthru->count = 0;
> bb = fallthru->dest;
>
> /* Redirect back edges we want to keep. */
> @@ -842,20 +845,13 @@ make_forwarder_block (basic_block bb, bool (*redir
>
> if (redirect_edge_p (e))
> {
> + dummy->frequency += EDGE_FREQUENCY (e);
> + dummy->count += e->count;
> + fallthru->count += e->count;
> ei_next (&ei);
> continue;
> }
>
> - dummy->frequency -= EDGE_FREQUENCY (e);
> - dummy->count -= e->count;
> - if (dummy->frequency < 0)
> - dummy->frequency = 0;
> - if (dummy->count < 0)
> - dummy->count = 0;
> - fallthru->count -= e->count;
> - if (fallthru->count < 0)
> - fallthru->count = 0;
> -
Can you elaborate why these changes are needed? They look unrelated,
so it is a bug in forwarder block profile updating?
I do not see why source of the edge needs to have profile cleaned.
> Index: gcc/loop-unroll.c
> ===================================================================
> --- gcc/loop-unroll.c (revision 210180)
> +++ gcc/loop-unroll.c (working copy)
> @@ -998,6 +998,13 @@ decide_unroll_runtime_iterations (struct loop *loo
> return;
> }
>
> + /* In AutoFDO, the profile is not accurate. If the calculated trip count
> + is larger than the header count, then the profile is not accurate
> + enough to make correct unroll decisions. */
> + if (flag_auto_profile
> + && expected_loop_iterations (loop) > loop->header->count)
> + return;
This is another independent change. It does make sense, but I would
preffer to have it in a separate function (expected_loop_iterations_reliable_p)
rather than inline in the code.
There are other loop optimizations that are driven by this, and they should
consistently use this predicate.
In fact current -fprofile-use enabled -funroll-loops flag that enables all
unrolling independently of reliablility of the profile. I suppose we need to
imporove this scheme and have something liek unroll-loop-with-reliable-profile
path that would default for profile feedback optimization.
I also do not think your check makes a lot of sense. You want to compare loop
header count and loop preheader or so, or this is not reliable enough for
auto-FDO profiles?
In any case, lets handle this separately of the main autoFDO patches (and not
forget on it).
> Index: gcc/dwarf2out.c
> ===================================================================
> --- gcc/dwarf2out.c (revision 210180)
> +++ gcc/dwarf2out.c (working copy)
> @@ -2481,6 +2481,40 @@ const struct gcc_debug_hooks dwarf2_debug_hooks =
> 1, /* start_end_main_source_file */
> TYPE_SYMTAB_IS_DIE /* tree_type_symtab_field */
> };
> +
> +const struct gcc_debug_hooks auto_profile_debug_hooks =
> +{
> + debug_nothing_charstar,
> + debug_nothing_charstar,
> + debug_nothing_void,
> + debug_nothing_int_charstar,
> + debug_nothing_int_charstar,
> + debug_nothing_int_charstar,
> + debug_nothing_int,
> + debug_nothing_int_int, /* begin_block */
> + debug_nothing_int_int, /* end_block */
> + dwarf2out_ignore_block, /* ignore_block */
> + debug_nothing_int_charstar_int_bool, /* source_line */
> + debug_nothing_int_charstar, /* begin_prologue */
> + debug_nothing_int_charstar, /* end_prologue */
> + debug_nothing_int_charstar, /* begin_epilogue */
> + debug_nothing_int_charstar, /* end_epilogue */
> + debug_nothing_tree, /* begin_function */
> + debug_nothing_int, /* end_function */
> + debug_nothing_tree, /* function_decl */
> + debug_nothing_tree, /* global_decl */
> + debug_nothing_tree_int, /* type_decl */
> + debug_nothing_tree_tree_tree_bool, /* imported_module_or_decl */
> + debug_nothing_tree, /* deferred_inline_function */
> + debug_nothing_tree, /* outlining_inline_function */
> + debug_nothing_rtx, /* label */
> + debug_nothing_int, /* handle_pch */
> + debug_nothing_rtx, /* var_location */
> + debug_nothing_void, /* switch_text_section */
> + debug_nothing_tree_tree, /* set_name */
> + 0, /* start_end_main_source_file */
> + TYPE_SYMTAB_IS_ADDRESS /* tree_type_symtab_field */
> +};
Note that I can not approve changes to debug machine (and do not feel confident
in it), so lets do this as independent change, too.
You need those to reliably output line info with autofdo enabled or why this
is needed at all?
>
> /* NOTE: In the comments in this file, many references are made to
> "Debugging Information Entries". This term is abbreviated as `DIE'
> @@ -16799,10 +16833,9 @@ add_src_coords_attributes (dw_die_ref die, tree de
> static void
> add_linkage_name (dw_die_ref die, tree decl)
> {
> - if (debug_info_level > DINFO_LEVEL_TERSE
> + if (debug_info_level > DINFO_LEVEL_NONE
> && (TREE_CODE (decl) == FUNCTION_DECL || TREE_CODE (decl) == VAR_DECL)
> && TREE_PUBLIC (decl)
> - && !DECL_ABSTRACT (decl)
> && !(TREE_CODE (decl) == VAR_DECL && DECL_REGISTER (decl))
> && die->die_tag != DW_TAG_member)
> {
> /* Counters that are collected. */
> Index: gcc/cfg-flags.def
> ===================================================================
> --- gcc/cfg-flags.def (revision 210180)
> +++ gcc/cfg-flags.def (working copy)
> @@ -93,6 +93,9 @@ DEF_BASIC_BLOCK_FLAG(VISITED, 13)
> demand, and is available after calling compute_transaction_bits(). */
> DEF_BASIC_BLOCK_FLAG(IN_TRANSACTION, 14)
>
> +/* Set on blocks that has been annotated during AutoFDO profile
> + attribution. */
> +DEF_BASIC_BLOCK_FLAG(ANNOTATED, 15)
ANNOTATED is uninformative name, perhaps AUTOFDO_ANNOTATED?
What is the annotation used for?
> Index: gcc/ira-int.h
> ===================================================================
> --- gcc/ira-int.h (revision 210180)
> +++ gcc/ira-int.h (working copy)
> @@ -41,9 +41,10 @@ along with GCC; see the file COPYING3. If not see
> analogous to REG_FREQ_FROM_BB. When optimizing for size, or
> profile driven feedback is available and the function is never
> executed, frequency is always equivalent. Otherwise rescale the
> - edge frequency. */
> + edge frequency. For AutoFDO, even if a function is not sampled,
> + it can still be executed, thus frequency rescale is still used. */
> #define REG_FREQ_FROM_EDGE_FREQ(freq)
> \
> - (optimize_size || (flag_branch_probabilities
> \
> + (optimize_size || (flag_branch_probabilities && !flag_auto_profile \
> && !ENTRY_BLOCK_PTR_FOR_FN (cfun)->count) \
> ? REG_FREQ_MAX : (freq * REG_FREQ_MAX / BB_FREQ_MAX) \
> ? (freq * REG_FREQ_MAX / BB_FREQ_MAX) : 1)
This also needs more consistent solution.
I think this macro is out of date and hsould test
optimize_function_for_size_p predicate and the predicate should do the right
thing for auto-FDO.
This change is pre-approved for mainline.
> Index: gcc/tree-profile.c
> ===================================================================
> --- gcc/tree-profile.c (revision 210180)
> +++ gcc/tree-profile.c (working copy)
> @@ -696,7 +696,7 @@ bool
> pass_ipa_tree_profile::gate (function *)
> {
> /* When profile instrumentation, use or test coverage shall be performed.
> */
> - return (!in_lto_p
> + return (!in_lto_p && !flag_auto_profile
> && (flag_branch_probabilities || flag_test_coverage
> || profile_arc_flag));
Update comment here.
> }
> Index: gcc/tree-inline.c
> ===================================================================
> --- gcc/tree-inline.c (revision 210180)
> +++ gcc/tree-inline.c (working copy)
> @@ -1977,9 +1977,10 @@ copy_edges_for_bb (basic_block bb, gcov_type count
> edge new_edge;
>
> flags = old_edge->flags;
> + flags &= (~EDGE_ANNOTATED);
Probably comment on why you want to clear annotations.
> @@ -2191,6 +2192,9 @@ initialize_cfun (tree new_fndecl, tree callee_fnde
> else
> count_scale = REG_BR_PROB_BASE;
>
> + if (count_scale > REG_BR_PROB_BASE)
> + count_scale = REG_BR_PROB_BASE;
> +
> /* Register specific tree functions. */
> gimple_register_cfg_hooks ();
>
> @@ -2452,6 +2456,9 @@ copy_cfg_body (copy_body_data * id, gcov_type coun
> else
> count_scale = REG_BR_PROB_BASE;
>
> + if (count_scale > REG_BR_PROB_BASE)
> + count_scale = REG_BR_PROB_BASE;
> +
> /* Register specific tree functions. */
> gimple_register_cfg_hooks ();
>
These changes also looks independent and should go in separately (with an
explanation)
> Index: gcc/bb-reorder.c
> ===================================================================
> --- gcc/bb-reorder.c (revision 210180)
> +++ gcc/bb-reorder.c (working copy)
> @@ -2663,7 +2663,7 @@ pass_partition_blocks::gate (function *fun)
> user defined section attributes. Don't call it if either case
> arises. */
> return (flag_reorder_blocks_and_partition
> - && optimize
> + && optimize &&!flag_auto_profile
Formatting error. Why we want !flag_auto_profile? Can't we just arrange
flag_reorder_blocks_and_partition to default to false?
In fact I would like to see flag_reorder_blocks_and_partition to offload
obviously
cold code (in partiuclar the EH cleanups) even without profile. That should work
well with autofdo.
> Index: gcc/coverage.c
> ===================================================================
> --- gcc/coverage.c (revision 210180)
> +++ gcc/coverage.c (working copy)
> @@ -54,6 +54,7 @@ along with GCC; see the file COPYING3. If not see
> #include "intl.h"
> #include "filenames.h"
> #include "target.h"
> +#include "auto-profile.h"
>
> #include "gcov-io.h"
> #include "gcov-io.c"
> @@ -1175,7 +1176,9 @@ coverage_init (const char *filename)
>
> bbg_file_stamp = local_tick;
>
> - if (flag_branch_probabilities)
> + if (flag_auto_profile)
> + init_auto_profile ();
Perhaps initialization can happen from toplev.c instead?
> Index: gcc/profile.c
> ===================================================================
> --- gcc/profile.c (revision 210180)
> +++ gcc/profile.c (working copy)
> @@ -106,6 +106,12 @@ static int total_num_times_called;
> static int total_hist_br_prob[20];
> static int total_num_branches;
>
> +void add_working_set (gcov_working_set_t *set) {
> + int i = 0;
> + for (; i < NUM_GCOV_WORKING_SETS; i++)
> + gcov_working_sets[i] = set[i];
> +}
Comment here.
> Index: gcc/regs.h
> ===================================================================
> --- gcc/regs.h (revision 210180)
> +++ gcc/regs.h (working copy)
> @@ -137,6 +137,7 @@ extern size_t reg_info_p_size;
> frequency. */
> #define REG_FREQ_FROM_BB(bb) (optimize_size \
> || (flag_branch_probabilities \
> + && !flag_auto_profile \
> && !ENTRY_BLOCK_PTR_FOR_FN (cfun)->count) \
> ? REG_FREQ_MAX \
> : ((bb)->frequency * REG_FREQ_MAX / BB_FREQ_MAX)\
Again, I think optimize_function_for_size is good predicate here.
> Index: gcc/common.opt
> ===================================================================
> --- gcc/common.opt (revision 210180)
> +++ gcc/common.opt (working copy)
> @@ -878,6 +878,16 @@ fauto-inc-dec
> Common Report Var(flag_auto_inc_dec) Init(1)
> Generate auto-inc/dec instructions
>
> +fauto-profile
> +Common Report Var(flag_auto_profile) Optimization
> +Use sample profile information for call graph node weights. The default
> +profile file is fbdata.afdo in 'pwd'.
> +
> +fauto-profile=
> +Common Joined RejectNegative Var(auto_profile_file)
> +Use sample profile information for call graph node weights. The profile
> +file is specified in the argument.
I believe auto-fdo will need a separate section in the manual describing the
whole machinery.
> Index: gcc/opts.c
> ===================================================================
> --- gcc/opts.c (revision 210180)
> +++ gcc/opts.c (working copy)
> @@ -627,6 +627,11 @@ default_options_optimization (struct gcc_options *
> default_param_value (PARAM_MIN_CROSSJUMP_INSNS),
> opts->x_param_values, opts_set->x_param_values);
>
> + if (flag_auto_profile)
> + maybe_set_param_value
> + (PARAM_EARLY_INLINER_MAX_ITERATIONS, 10,
> + opts->x_param_values, opts_set->x_param_values);
> +
> /* Allow default optimizations to be specified on a per-machine basis. */
> maybe_default_options (opts, opts_set,
> targetm_common.option_optimization_table,
> @@ -1720,6 +1725,56 @@ common_handle_option (struct gcc_options *opts,
> opts->x_flag_devirtualize_speculatively = false;
> break;
>
> + case OPT_fauto_profile_:
> + opts->x_auto_profile_file = xstrdup (arg);
> + opts->x_flag_auto_profile = true;
> + value = true;
> + /* No break here - do -fauto-profile processing. */
> + case OPT_fauto_profile:
> + if (!opts_set->x_flag_branch_probabilities)
> + opts->x_flag_branch_probabilities = value;
> + if (!opts_set->x_flag_profile_values)
> + opts->x_flag_profile_values = value;
> + if (!opts_set->x_flag_unroll_loops)
> + opts->x_flag_unroll_loops = value;
> + if (!opts_set->x_flag_peel_loops)
> + opts->x_flag_peel_loops = value;
> + if (!opts_set->x_flag_tracer)
> + opts->x_flag_tracer = value;
> + if (!opts_set->x_flag_value_profile_transformations)
> + opts->x_flag_value_profile_transformations = value;
> + if (!opts_set->x_flag_inline_functions)
> + opts->x_flag_inline_functions = value;
> + if (!opts_set->x_flag_ipa_cp)
> + opts->x_flag_ipa_cp = value;
> + if (!opts_set->x_flag_ipa_cp_clone
> + && value && opts->x_flag_ipa_cp)
> + opts->x_flag_ipa_cp_clone = value;
> + if (!opts_set->x_flag_predictive_commoning)
> + opts->x_flag_predictive_commoning = value;
> + if (!opts_set->x_flag_unswitch_loops)
> + opts->x_flag_unswitch_loops = value;
> + if (!opts_set->x_flag_gcse_after_reload)
> + opts->x_flag_gcse_after_reload = value;
> + if (!opts_set->x_flag_tree_loop_vectorize
> + && !opts_set->x_flag_tree_vectorize)
> + opts->x_flag_tree_loop_vectorize = value;
> + if (!opts_set->x_flag_tree_slp_vectorize
> + && !opts_set->x_flag_tree_vectorize)
> + opts->x_flag_tree_slp_vectorize = value;
> + if (!opts_set->x_flag_vect_cost_model)
> + opts->x_flag_vect_cost_model = VECT_COST_MODEL_DYNAMIC;
> + if (!opts_set->x_flag_tree_loop_distribute_patterns)
> + opts->x_flag_tree_loop_distribute_patterns = value;
> + /* Indirect call profiling should do all useful transformations
> + speculative devirutalization does. */
> + if (!opts_set->x_flag_devirtualize_speculatively
> + && opts->x_flag_value_profile_transformations)
> + opts->x_flag_devirtualize_speculatively = false;
> + if (!opts_set->x_flag_profile_correction)
> + opts->x_flag_profile_correction = value;
> + break;
Instead of cut&paste from profile-use I would go for function setting the
common flags.
> Index: gcc/tree-cfg.c
> ===================================================================
> --- gcc/tree-cfg.c (revision 210180)
> +++ gcc/tree-cfg.c (working copy)
> @@ -1877,6 +1877,14 @@ gimple_merge_blocks (basic_block a, basic_block b)
> }
> }
>
> + /* When merging two BBs, if their counts are different, the larger
> + count is selected as the new bb count. */
> + if (flag_auto_profile && a->count != b->count)
> + {
> + a->count = MAX (a->count, b->count);
> + a->frequency = MAX (a->frequency, b->frequency);
> + }
Separate change and OK for mainline. (perhaps with comment update
that this if to handle better inconsistent profiles)
> +
> /* Merge the sequences. */
> last = gsi_last_bb (a);
> gsi_insert_seq_after (&last, bb_seq (b), GSI_NEW_STMT);
> Index: gcc/cgraphclones.c
> ===================================================================
> --- gcc/cgraphclones.c (revision 210180)
> +++ gcc/cgraphclones.c (working copy)
> @@ -435,6 +435,11 @@ cgraph_clone_node (struct cgraph_node *n, tree dec
> }
> else
> count_scale = 0;
> + /* In AutoFDO, if edge count is larger than callee's entry block
> + count, we will not update the original callee because it may
> + mistakenly mark some hot function as cold. */
> + if (flag_auto_profile && count >= n->count)
> + update_original = false;
This looks like a hack. Lets go with it later - I see what you are shooting for
here
and non-autoFDO has kind of same problem, too. Counts are not at all that
reliable
after some inlining.
I wonder how this hack would fare for -fprofile-use
> @@ -1286,6 +1285,9 @@ del_node_map (void)
> struct cgraph_node*
> find_func_by_profile_id (int profile_id)
> {
> + if (flag_auto_profile)
> + return cgraph_node_for_asm (
> + get_identifier ((const char *)(size_t)profile_id));
I think with LTO we will need to make this machinery safe for symbol mangling.
Any plans on this?
> @@ -1492,8 +1494,8 @@ gimple_ic_transform (gimple_stmt_iterator *gsi)
> /* The order of CHECK_COUNTER calls is important -
> since check_counter can correct the third parameter
> and we want to make count <= all <= bb_all. */
> - if ( check_counter (stmt, "ic", &all, &bb_all, bb_all)
> - || check_counter (stmt, "ic", &count, &all, all))
> + if (!flag_auto_profile && (check_counter (stmt, "ic", &all, &bb_all,
> bb_all)
> + || check_counter (stmt, "ic", &count, &all, all)))
Why you skip check for auto profile here, but not for other transforms?
Similar check is now done at ipa-profile later. I suppose you may want to make
autofdo
code to make the counters just look right when they are supposed to be right.
> Index: gcc/auto-profile.c
> ===================================================================
> --- gcc/auto-profile.c (revision 0)
> +++ gcc/auto-profile.c (revision 0)
> @@ -0,0 +1,1584 @@
> +/* Calculate branch probabilities, and basic block execution counts.
> + Copyright (C) 2012. Free Software Foundation, Inc.
> + Contributed by Dehao Chen (de...@google.com)
I will look into this file incrementally. You want to update copyright year.
> Index: gcc/ipa-inline.c
> ===================================================================
> --- gcc/ipa-inline.c (revision 210180)
> +++ gcc/ipa-inline.c (working copy)
> @@ -121,6 +121,7 @@ along with GCC; see the file COPYING3. If not see
> #include "ipa-inline.h"
> #include "ipa-utils.h"
> #include "sreal.h"
> +#include "auto-profile.h"
> #include "cilk.h"
>
> /* Statistics we collect about inlining algorithm. */
> @@ -450,6 +451,8 @@ want_early_inline_function_p (struct cgraph_edge *
>
> if (DECL_DISREGARD_INLINE_LIMITS (callee->decl))
> ;
> + else if (flag_auto_profile && afdo_callsite_hot_enough_for_early_inline
> (e))
> + ;
You want to explain what happens here (I remember it from presentation :)
> Index: gcc/ipa-inline.h
> ===================================================================
> --- gcc/ipa-inline.h (revision 210180)
> +++ gcc/ipa-inline.h (working copy)
> @@ -345,3 +345,5 @@ reset_edge_growth_cache (struct cgraph_edge *edge)
> edge_growth_cache[edge->uid] = zero;
> }
> }
> +
> +unsigned int early_inliner (function *fun);
Can't this be handled by scheduling early inliner as subpass of autofdo or
something similar?
I would rather make this explicit in PM.
Honza
