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