On Mon, May 6, 2019 at 9:59 AM Martin Liška <mli...@suse.cz> wrote:
>
> On 5/2/19 2:31 PM, Richard Biener wrote:
> > On Mon, Apr 29, 2019 at 2:51 PM Martin Liška <mli...@suse.cz> wrote:
> >>
> >> On 4/26/19 3:18 PM, Richard Biener wrote:
> >>> On Wed, Apr 10, 2019 at 10:12 AM Martin Liška <mli...@suse.cz> wrote:
> >>>>
> >>>> On 4/9/19 3:19 PM, Jan Hubicka wrote:
> >>>>>> Hi.
> >>>>>>
> >>>>>> There's updated version that supports profile quality for both counts
> >>>>>> and probabilities. I'm wondering whether ENTRY and EXIT BBs needs to
> >>>>>> have set probability. Apparently, I haven't seen any verifier that
> >>>>>> would complain.
> >>>>>
> >>>>> Well, you do not need to define it but then several cases will
> >>>>> degenerate. In particular BB frequencies (for callgraph profile or
> >>>>> hot/cold decisions) are calculated as ratios of entry BB and given BB
> >>>>> count. If entry BB is undefined you will get those undefined and
> >>>>> heuristics will resort to conservative answers.
> >>>>>
> >>>>> I do not think we use exit block count.
> >>>>>
> >>>>> Honza
> >>>>>
> >>>>
> >>>> Thank you Honza for explanation. I'm sending version of the patch
> >>>> that supports entry BB count.
> >>>>
> >>>> I've been testing the patch right now.
> >>>
> >>> Can you move the GIMPLE/RTL FE specific data in c_declspecs to
> >>> a substructure accessed via indirection? I guess enlarging that
> >>> isn't really what we should do. You'd move gimple_or_rtl_pass
> >>> there and make that pointer one to a struct aux_fe_data
> >>> (lifetime managed by the respective RTL/GIMPLE FE, thus
> >>> to be freed there)? Joseph, do you agree or is adding more
> >>> stuff to c_declspecs OK (I would guess it could be a few more
> >>> elements in the future).
> >>
> >> Let's wait here for Joseph.
> >
> > So looks like it won't matter so let's go with the current approach
> > for the moment.
> >
> >>>
> >>> -c_parser_gimple_parse_bb_spec (tree val, int *index)
> >>> +c_parser_gimple_parse_bb_spec (tree val, gimple_parser &parser,
> >>> + int *index, profile_probability
> >>> *probablity)
> >>> {
> >>>
> >>> so this will allow specifying probability in PHI node arguments.
> >>> I think we want to split this into the already existing part
> >>> and a c_parser_gimple_parse_bb_spec_with_edge_probability
> >>> to be used at edge sources.
> >>
> >> Yes, that's a good idea!
> >>
> >>>
> >>> + if (cfun->curr_properties & PROP_cfg)
> >>> + {
> >>> + update_max_bb_count ();
> >>> + set_hot_bb_threshold (hot_bb_threshold);
> >>> + ENTRY_BLOCK_PTR_FOR_FN (cfun)->count = entry_bb_count;
> >>>
> >>> I guess the last one should be before update_max_bb_count ()?
> >>
> >> Same here.
> >>
> >>>
> >>> + }
> >>>
> >>> + /* Parse profile: quality(value) */
> >>> else
> >>> {
> >>> - c_parser_error (parser, "unknown block specifier");
> >>> - return return_p;
> >>> + tree q;
> >>> + profile_quality quality;
> >>> + tree v = c_parser_peek_token (parser)->value;
> >>>
> >>> peek next token before the if/else and do
> >>>
> >>> else if (!strcmp (...))
> >>>
> >>> as in the loop_header case. I expected we can somehow share
> >>> parsing of profile quality and BB/edge count/frequency? How's
> >>> the expected syntax btw, comments in the code should tell us.
> >>> I'm guessing it's quality-id '(' number ')' and thus it should be
> >>> really shareable between edge and BB count and also __GIMPLE
> >>> header parsing? So parse_profile_quality should be
> >>> parse_profile () instead, resulting in a combined value
> >>> (we do use the same for edge/bb?).
> >>
> >> It's problematic, there are different error messages for count/frequency.
> >> Moreover call to parse_profile_quality in c_parser_gimple_or_rtl_pass_list
> >> is a way how to test that next 'token' is a profile count.
> >
> > Who cares about error messages... But sure, I'm just proposing to
> > merge testing for next token and actual parsing.
>
> After I've done removal of hot_bb_threshold parsing, there are just 2 usages
> of parse_profile_quality. I would like to leave it as it, not introducing a
> wrappers.
>
> >
> >>>
> >>> + else if (!strcmp (op, "hot_bb_threshold"))
> >>> + {
> >>>
> >>> I'm not sure about this - it doesn't make sense to specify this
> >>> on a per-function base since it seems to control a global
> >>> variable (booo!)?
> >>
> >> Yep, shame on me!
> >>
> >>> Isn't this instead computed on-demand
> >>> based on profile_info->sum_max?
> >>
> >> No it's a global value shared among functions.
> >>
> >>> If not then I think
> >>> we need an alternate way of funneling in global state into
> >>> the GIMPLE FE.
> >>
> >> What about --param gimple-fe-hot-bb-threshold ?
> >
> > I thought about that, yes ... in absence can it actually be
> > "computed"?
>
> Renamed to it.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
@@ -470,7 +576,8 @@ c_parser_gimple_compound_statement (gimple_parser
&parser, gimple_seq *seq)
last_basic_block_for_fn (cfun) = index + 1;
n_basic_blocks_for_fn (cfun)++;
if (!parser.current_bb)
- parser.push_edge (ENTRY_BLOCK, bb->index, EDGE_FALLTHRU);
+ parser.push_edge (ENTRY_BLOCK, bb->index, EDGE_FALLTHRU,
+ profile_probability::uninitialized ());
/* We leave the proper setting to fixup. */
struct loop *loop_father = loops_for_fn (cfun)->tree_root;
the fallthru edge from ENTRY to the single-succ of ENTRY should have
100% probability and be "known", no? Or do we actually use uninitialized
for fallthru edges?
diff --git a/gcc/params.def b/gcc/params.def
index 3c9c5fc0f13..840b81f43cc 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1414,6 +1414,12 @@ DEFPARAM(PARAM_LOOP_VERSIONING_MAX_OUTER_INSNS,
" loops.",
100, 0, 0)
+DEFPARAM(PARAM_GIMPLE_FE_COMPUTED_HOT_BB_THRESHOLD,
+ "gimple-fe-computed-hot-bb-threshold",
+ "The number of executions of a basic block which is considered hot."
+ " The parameters is used only in GIMPLE FE.",
+ 0, 0, 0)
+
/*
Local variables:
diff --git a/gcc/predict.c b/gcc/predict.c
index b010c20ff7d..12a484a799a 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -132,8 +132,11 @@ get_hot_bb_threshold ()
{
if (min_count == -1)
{
- min_count
- = profile_info->sum_max / PARAM_VALUE (HOT_BB_COUNT_FRACTION);
+ if (flag_gimple)
+ min_count = PARAM_VALUE (PARAM_GIMPLE_FE_COMPUTED_HOT_BB_THRESHOLD);
+ else
+ min_count
+ = profile_info->sum_max / PARAM_VALUE (HOT_BB_COUNT_FRACTION);
if (dump_file)
fprintf (dump_file, "Setting hotness threshold to %" PRId64 ".\n",
min_count);
Ick. I'd rather do this somewhere in the frontend where PARAM_VALUE
is initialized
via set_hot_bb_threshold. Even redundantly for each parsed GIMPLE
function would
be OK and better IMHO. That way you don't need get_current_hot_bb_threshold(?).
OK with that changes.
We need to think about switch stmt parsing where we currently have
switch (argc_2(D)) {default: L2; case 0: L0; case 1: L0; case 2: L1; }
__BB(3):
L0:
a_4 = 0;
goto __BB6;
...
extending this to
switch (argc_2(D)) { default: L2 (guessed(10)); ...
might be possible so we have profile on the edges.
After the patch all -gimple dumps have profile - probably good
but also visually disturbing. Ah well.
Hope to do loop flags as followup soon.
Thanks,
Richard.
> Thanks,
> Martin
>
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
> >>
> >>>
> >>> Thanks,
> >>> Richard.
> >>>
> >>>
> >>>>
> >>>> Martin
> >>
>
