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).
-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.
+ 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 ()?
+ }
+ /* 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?).
+ 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!)? Isn't this instead computed on-demand
based on profile_info->sum_max? If not then I think
we need an alternate way of funneling in global state into
the GIMPLE FE.
Thanks,
Richard.
>
> Martin
