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.
> >
> > + 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"?
Richard.
> Thanks,
> Martin
>
> >
> > Thanks,
> > Richard.
> >
> >
> >>
> >> Martin
>
