PING^2
On 5/24/22 13:35, Martin Liška wrote:
> PING^1
>
> On 5/5/22 20:15, Martin Liška wrote:
>> 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
>>
>
