From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf2e.google.com (mail-qv1-xf2e.google.com [IPv6:2607:f8b0:4864:20::f2e]) by sourceware.org (Postfix) with ESMTPS id D38893857C56 for ; Thu, 6 May 2021 13:33:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D38893857C56 Received: by mail-qv1-xf2e.google.com with SMTP id dl3so3020411qvb.3 for ; Thu, 06 May 2021 06:33:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=9SNJaq4eS9b6CGj9kf/Wjh1UfwyJShqhfHthjcUYJFY=; b=eydtOIKyad///WDO1iM3wLiEy6eUZ0ltLo0lQgy+Y4SKyS9BmgspoTgVBswOLS95iK B2v8T2W360nADmHmO2fmf4xkP326ks/esIiKqzCAgZaOkZ/9nsvRWGAIid0wCguhebsa Z3BY3iZizIsBUylsBz5P4weHW9RZVxY9o1klpb9/xGCQhatzFYK/6Fcr0bi0oWhsg42B tZAW7/3z93QAG4d0ZPrKt6OkE7qbSK266T/nwkf0OWw+PIO40Dxr01lbK/rZjhGpT2rW bogkchxj4G+OtIoKDl5zf71Z+rF+Dnb4qI2m+SyNzPPVFdLfKY2sUGUcH9yc/bFHV8LH wVuA== X-Gm-Message-State: AOAM532yEDZJCb6haOj0KGu9p/QnZGrCCE71cwRaJvxPSpBE/6uvEyb4 G5IA69tfa4d+GsVgx6RpPq2LVQ== X-Google-Smtp-Source: ABdhPJwK4AOZmdiBGGDjjCMt4qLp+JHkWLCl2Y5vl7uwMlCACu/3x60DQ8Pb8vbV/RFxmfbxXC74QQ== X-Received: by 2002:ad4:4109:: with SMTP id i9mr4253923qvp.30.1620308036172; Thu, 06 May 2021 06:33:56 -0700 (PDT) Received: from [192.168.1.4] ([177.194.37.86]) by smtp.gmail.com with ESMTPSA id u126sm1991137qkd.80.2021.05.06.06.33.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 May 2021 06:33:54 -0700 (PDT) Subject: Re: [PATCH 1/4] Remove architecture specific sched_cpucount optimizations To: Paul Eggert , Florian Weimer , Adhemerval Zanella via Libc-alpha Cc: crrodriguez@opensuse.org References: <20210329182520.323665-1-adhemerval.zanella@linaro.org> <87a6p9dr9n.fsf@oldenburg.str.redhat.com> <61040ff8-caac-a3d9-91cc-9b445c4e98fd@cs.ucla.edu> From: Adhemerval Zanella Message-ID: Date: Thu, 6 May 2021 10:33:52 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <61040ff8-caac-a3d9-91cc-9b445c4e98fd@cs.ucla.edu> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 May 2021 13:33:59 -0000 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 +/* 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; +} int __sched_cpucount (size_t setsize, const cpu_set_t *setp) { int s = 0; - const __cpu_mask *p = setp->__bits; - const __cpu_mask *end = &setp->__bits[setsize / sizeof (__cpu_mask)]; - - while (p < end) - { - __cpu_mask l = *p++; - -#ifdef POPCNT - s += POPCNT (l); -#else - if (l == 0) - continue; - - _Static_assert (sizeof (l) == sizeof (unsigned int) - || sizeof (l) == sizeof (unsigned long) - || sizeof (l) == sizeof (unsigned long long), - "sizeof (__cpu_mask"); - if (sizeof (__cpu_mask) == sizeof (unsigned int)) - s += __builtin_popcount (l); - else if (sizeof (__cpu_mask) == sizeof (unsigned long)) - s += __builtin_popcountl (l); - else - s += __builtin_popcountll (l); -#endif - } - + for (int i = 0; i < setsize / sizeof (__cpu_mask); i++) + s += countbits (setp->__bits[i]); return s; } diff --git a/sysdeps/i386/i686/multiarch/sched_cpucount.c b/sysdeps/i386/i686/multiarch/sched_cpucount.c deleted file mode 100644 index 7db31b02f8..0000000000 --- a/sysdeps/i386/i686/multiarch/sched_cpucount.c +++ /dev/null @@ -1 +0,0 @@ -#include diff --git a/sysdeps/ia64/sched_cpucount.c b/sysdeps/ia64/sched_cpucount.c deleted file mode 100644 index 8440864b02..0000000000 --- a/sysdeps/ia64/sched_cpucount.c +++ /dev/null @@ -1,20 +0,0 @@ -/* Copyright (C) 2007-2021 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -#define POPCNT(l) __builtin_popcountl (l) - -#include diff --git a/sysdeps/powerpc/sched_cpucount.c b/sysdeps/powerpc/sched_cpucount.c deleted file mode 100644 index 8f00e3dbc8..0000000000 --- a/sysdeps/powerpc/sched_cpucount.c +++ /dev/null @@ -1,22 +0,0 @@ -/* Copyright (C) 2007-2021 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -#ifdef _ARCH_PWR5 -# define POPCNT(l) __builtin_popcountl (l) -#endif - -#include diff --git a/sysdeps/x86_64/multiarch/sched_cpucount.c b/sysdeps/x86_64/multiarch/sched_cpucount.c deleted file mode 100644 index 5180a11434..0000000000 --- a/sysdeps/x86_64/multiarch/sched_cpucount.c +++ /dev/null @@ -1,36 +0,0 @@ -/* Count bits in CPU set. x86-64 multi-arch version. - This file is part of the GNU C Library. - Copyright (C) 2008-2021 Free Software Foundation, Inc. - Contributed by Ulrich Drepper . - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -#include -#include "init-arch.h" - -#define __sched_cpucount static generic_cpucount -#include -#undef __sched_cpucount - -#define POPCNT(l) \ - ({ __cpu_mask r; \ - asm ("popcnt %1, %0" : "=r" (r) : "0" (l));\ - r; }) -#define __sched_cpucount static popcount_cpucount -#include -#undef __sched_cpucount - -libc_ifunc (__sched_cpucount, - CPU_FEATURE_USABLE (POPCNT) ? popcount_cpucount : generic_cpucount); diff --git a/sysdeps/x86_64/sched_cpucount.c b/sysdeps/x86_64/sched_cpucount.c deleted file mode 100644 index 5a27336d6d..0000000000 --- a/sysdeps/x86_64/sched_cpucount.c +++ /dev/null @@ -1,25 +0,0 @@ -/* Copyright (C) 2007-2021 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -#ifdef __amdfam10 -# define POPCNT(l) \ - ({ __cpu_mask r; \ - asm ("popcntq %1, %0" : "=r" (r) : "0" (l)); \ - r; }) -#endif - -#include