On 5/5/22 15:49, Jan Hubicka wrote:
> Hi,
>> The patch simplifies usage of the profile_{count,probability} types.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>
> The reason I intentionally did not add * and / to the original API was
> to detect situations where values that should be
> profile_count/profile_probability are stored into integers, since
> previous code used integers for everything.
>
> Having one to add apply_scale made him/her (mostly me :) to think if the
> value is really just a fixed scale or it it should be better converted
> to proper data type (count or probability).
>
> I guess now we completed the conversion so risk of this creeping in is
> relatively low and the code indeed looks better.
Yes, that's my impression as well that the profiling code is quite settled down.
> It will make it bit
> harder for me to backport jump threading profile updating fixes I plan
> for 12.2 but it should not be hard.
You'll manage ;)
>> diff --git a/gcc/cfgloopmanip.cc b/gcc/cfgloopmanip.cc
>> index b4357c03e86..a1ac1146445 100644
>> --- a/gcc/cfgloopmanip.cc
>> +++ b/gcc/cfgloopmanip.cc
>> @@ -563,8 +563,7 @@ scale_loop_profile (class loop *loop,
>> profile_probability p,
>>
>> /* Probability of exit must be 1/iterations. */
>> count_delta = e->count ();
>> - e->probability = profile_probability::always ()
>> - .apply_scale (1, iteration_bound);
>> + e->probability = profile_probability::always () / iteration_bound;
> However this is kind of example of the problem.
> iteration_bound is gcov_type so we can get overflow here.
typedef int64_t gcov_type;
and apply_scale takes int64_t types as arguments. Similarly the newly added
operators,
so how can that change anything?
> I guess we want to downgrade iteration_bound since it is always either 0
> or int.
>> diff --git a/gcc/tree-switch-conversion.cc b/gcc/tree-switch-conversion.cc
>> index e14b4e6c94a..cef26a9878e 100644
>> --- a/gcc/tree-switch-conversion.cc
>> +++ b/gcc/tree-switch-conversion.cc
>> @@ -1782,7 +1782,7 @@ switch_decision_tree::analyze_switch_statement ()
>> tree high = CASE_HIGH (elt);
>>
>> profile_probability p
>> - = case_edge->probability.apply_scale (1, (intptr_t) (case_edge->aux));
>> + = case_edge->probability / ((intptr_t) (case_edge->aux));
>
> I think the switch ranges may be also in risk of overflow?
>
> We could make operators to accept gcov_type or int64_t.
As explained, they do.
Cheers,
Martin
>
> Thanks,
> Honza
