Re: [PATCH] Add operators / and * for profile_{count,probability}.

["Martin Liška" <mliska@suse.cz>]

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