On 05/16/2018 09:47 AM, Alexander Monakov wrote:
> On Wed, 16 May 2018, Martin Liška wrote:
> 
>> Hi.
>>
>> I consider it handy sometimes to trigger just a single invocation of
>> an optimization driven by a debug counter. Doing that one needs to
>> be able to limit both lower and upper limit of a counter. It's implemented
>> in the patch.
> 
> I'd like to offer some non-reviewer comments on the patch (below)

Hi.

I always appreciate these comments!

> 
>> --- a/gcc/common.opt
>> +++ b/gcc/common.opt
>> @@ -1171,7 +1171,7 @@ List all available debugging counters with their limits and counts.
>>  
>>  fdbg-cnt=
>>  Common RejectNegative Joined Var(common_deferred_options) Defer
>> --fdbg-cnt=<counter>:<limit>[,<counter>:<limit>,...]	Set the debug counter limit.
>> +-fdbg-cnt=<counter>[:<lower_limit>]:<upper_limit>[,<counter>:<lower_limit>:<upper_limit>,...]	Set the debug counter limit.
> 
> This line has gotten quite long and repeating the same thing in the second
> brackets is not very helpful. Can we use something simpler like this?
> 
> -fdbg-cnt=<counter>[:<lower_limit>]:<upper_limit>[,<counter>:...]

Yes, it's a nice simplfication.

> 
>> +#define DEBUG_COUNTER(a) 1,
>> +static unsigned int limit_low[debug_counter_number_of_counters] =
>> +{
>> +#include "dbgcnt.def"
>> +};
>> +#undef DEBUG_COUNTER
>> +
>> +
>>  static unsigned int count[debug_counter_number_of_counters];
>>  
>>  bool
>>  dbg_cnt_is_enabled (enum debug_counter index)
>>  {
>> -  return count[index] <= limit[index];
>> +  return limit_low[index] <= count[index] && count[index] <= limit_high[index];
> 
> I recall Jakub recently applied a tree-wide change of A < B && B < C to read
> B > A && B < C.

Can you please point to a revision where it was done?

> 
> Please consider making limit_low non-inclusive by testing for strict inequality
> count[index] > limit_low[index]. This will allow to make limit_low[] array
> zero-initialized (taking up space only in BSS).

Sure, nice idea. I did it, now all -dbg-cnt values are zero-based. I consider it easier
to work with. And as the usage is quite internal I hope we can adjust the logic to be
zero-based.

> 
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -14326,13 +14326,14 @@ Print the name and the counter upper bound for all debug counters.
>>  
>>  @item -fdbg-cnt=@var{counter-value-list}
>>  @opindex fdbg-cnt
>> -Set the internal debug counter upper bound.  @var{counter-value-list}
>> -is a comma-separated list of @var{name}:@var{value} pairs
>> -which sets the upper bound of each debug counter @var{name} to @var{value}.
>> +Set the internal debug counter lower and upper bound.  @var{counter-value-list}
>> +is a comma-separated list of @var{name}:@var{lower_bound}:@var{upper_bound}
>> +tuples which sets the lower and the upper bound of each debug
>> +counter @var{name}.
> 
> Shouldn't this mention that lower bound is optional?

Yes, fixed.

> 
>>  All debug counters have the initial upper bound of @code{UINT_MAX};
>>  thus @code{dbg_cnt} returns true always unless the upper bound
>>  is set by this option.
>> -For example, with @option{-fdbg-cnt=dce:10,tail_call:0},
>> +For example, with @option{-fdbg-cnt=dce:9:10,tail_call:0},
>>  @code{dbg_cnt(dce)} returns true only for first 10 invocations.
> 
> This seems confusing, you added a lower bound to the 'dce' counter,
> but the following text remains unchanged and says it's enabled for
> first 10 calls?

Yes, now fixed.

Thanks again,
Martin

> 
> Alexander
>