Re: [PATCH 1/4] Remove architecture specific sched_cpucount optimizations

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 06/05/2021 10:43, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 05/05/2021 15:25, Paul Eggert wrote:
>>> On 5/5/21 10:28 AM, Florian Weimer via Libc-alpha wrote:
>>>>> diff --git a/posix/sched_cpucount.c b/posix/sched_cpucount.c
>>>>> index b0ca4ea7bc..529286e777 100644
>>>>> --- a/posix/sched_cpucount.c
>>>>> +++ b/posix/sched_cpucount.c
>>>>> @@ -22,31 +22,11 @@ int
>>>>>   __sched_cpucount (size_t setsize, const cpu_set_t *setp)
>>>>>   {
>>>>>     int s = 0;
>>>>> +  for (int i = 0; i < setsize / sizeof (__cpu_mask); i++)
>>>>>       {
>>>>> +      __cpu_mask si = setp->__bits[i];
>>>>> +      /* Clear the least significant bit set.  */
>>>>> +      for (; si != 0; si &= si - 1, s++);
>>>>>       }
>>>>> -
>>>>>     return s;
>>>>>   }
>>>> Why “si”?  It think si &= si - 1 clears the*most*  significant bit in
>>>> si.  If you agree, please update the comment.
>>>
>>> Better yet, define a static function 'popcount' that uses Kernighan's trick and call that function. As things stand it's not obvious what the code is doing, regardless of which bit it's clearing. The function's comment should explain why it's not using __builtin_popcount.
>>
>> What about the below:
>>
>> diff --git a/posix/sched_cpucount.c b/posix/sched_cpucount.c
>> index b0ca4ea7bc..6cb5c4337e 100644
>> --- a/posix/sched_cpucount.c
>> +++ b/posix/sched_cpucount.c
>> @@ -17,36 +17,21 @@
>>  
>>  #include <sched.h>
>>  
>> +/* Counting bits set, Brian Kernighan's way  */
>> +static inline unsigned int
>> +countbits (__cpu_mask v)
>> +{
>> +  unsigned int s = 0;
>> +  for (; v != 0; s++)
>> +    v &= v - 1;
>> +  return s;
>> +}
> 
> I get that choosing the exact matching builtin for the __cpu_mask type
> isn't easy, but wouldn't it be safe to use __builtin_popcountll
> unconditionally?

Using a open-coded routine is slight better for architectures that
do not have a popcount instruction, since __builtin_popcountll will
call the libgcc routine).  But I hardly think we need that amount
of micro-optimization for such routine.