Post by Dehao ChenHi,
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,
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.
Post by Dehao ChenIndex: 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.
Post by Dehao ChenIndex: 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).
Post by Dehao ChenIndex: 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?
Post by Dehao Chen/* 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?
Post by Dehao ChenIndex: 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.
Post by Dehao ChenIndex: 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.
Post by Dehao Chen}
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.
Post by Dehao Chen@@ -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)
Post by Dehao ChenIndex: 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.
Post by Dehao ChenIndex: 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?
Post by Dehao ChenIndex: 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.
Post by Dehao ChenIndex: 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.
Post by Dehao ChenIndex: 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.
Post by Dehao ChenIndex: 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;
+ opts->x_auto_profile_file = xstrdup (arg);
+ opts->x_flag_auto_profile = true;
+ value = true;
+ /* No break here - do -fauto-profile processing. */
+ 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.
Post by Dehao ChenIndex: 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)
Post by Dehao Chen+
/* 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
Post by Dehao Chen@@ -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?
Post by Dehao Chen@@ -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.
Post by Dehao ChenIndex: 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.
I will look into this file incrementally. You want to update copyright year.
Post by Dehao ChenIndex: 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 :)
Post by Dehao ChenIndex: 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