On Tue, May 7, 2019 at 2:00 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 5/6/19 4:02 PM, Richard Biener wrote:
> > 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?
>
> Fixed by setting to ::always ().
>
> >
> > 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.
>
> I've done that in c_parser_parse_gimple_body.
>
> > That way you don't need get_current_hot_bb_threshold(?).
>
> We still want it for dump_function_to_file that prints the value in a comment.
But that can use the existing get_hot_bb_threshold since we never want
to dump -1 in case min_count was never initialized.
> >
> > 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.
>
> I can work on that during this stage1.
>
> Martin
>
> >
> > 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
> >>>>
> >>
>
