Use __builtin_popcount in __sched_cpucount [BZ #21696]

Message ID 20170701141209.GA18342@gmail.com
State New, archived
Headers

Commit Message

H.J. Lu July 1, 2017, 2:12 p.m. UTC
  On Sat, Jul 01, 2017 at 08:33:19AM +0200, Florian Weimer wrote:
> On 07/01/2017 03:14 AM, H.J. Lu wrote:
> > posix/sched_cpucount.c assumes that size of __cpu_mask == size of long,
> > which is incorrect for x32.  This patch generates size of __cpu_mask and
> > uses it in posix/sched_cpucount.c.
> > 
> > Tested on i686, x86-64 and x32 with multi-arch disabled.
> > 
> > OK for master?
> > 
> > H.J.
> > 	[BZ #21696]
> > 	* posix/Makefile (gen-as-const-headers): New.
> > 	* posix/cpu_mask.sym: New file.
> > 	* posix/sched_cpucount.c: Include <cpu_mask.h>.
> > 	(__sched_cpucount): Check CPU_MASK_SIZE instead of LONG_BIT.
> > 	Replace the ul suffix with the ull suffix for 64-bit cpu_mask.
> 
> Does one of the existing test cases fail reliably due to this?
> 

Yes, posix/tst-cpuset and posix/tst-cpucount fail when multi-arch is
disabled or the processor doesn't have popcnt.  Otherwise, popcnt
instruction is used.

> > +gen-as-const-headers = cpu_mask.sym
> 
> This is not needed if you use a regular #if and not a preprocessor
> conditional.
> 
>   _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);
> 
> Untested, but __builtin_popcount… has fallback emulation GCC, so this
> work.  (And GCC 4.8 already has it.)
> 

Thanks.  It works.  Here is the updated patch.  Tested on i686, x86-64
and x32 with multi-arch disabled.  OK for master?

Thanks.

H.J.
----
posix/sched_cpucount.c assumes that size of __cpu_mask == size of long,
which is incorrect for x32.  This patch uses __builtin_popcount, which
is availabe in GCC 4.9, in posix/sched_cpucount.c.

Tested on i686, x86-64 and x32 with multi-arch disabled.

2017-07-01  Florian Weimer  <fweimer@redhat.com>
	    H.J. Lu  <hongjiu.lu@intel.com>

	[BZ #21696]
	* posix/sched_cpucount.c: Don't include <limits.h>.
	(__sched_cpucount): Use __builtin_popcount.
---
 posix/sched_cpucount.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)
  

Comments

Florian Weimer July 1, 2017, 2:26 p.m. UTC | #1
On 07/01/2017 04:12 PM, H.J. Lu wrote:
> posix/sched_cpucount.c assumes that size of __cpu_mask == size of long,
> which is incorrect for x32.  This patch uses __builtin_popcount, which
> is availabe in GCC 4.9, in posix/sched_cpucount.c.
> 
> Tested on i686, x86-64 and x32 with multi-arch disabled.
> 
> 2017-07-01  Florian Weimer  <fweimer@redhat.com>
> 	    H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	[BZ #21696]
> 	* posix/sched_cpucount.c: Don't include <limits.h>.
> 	(__sched_cpucount): Use __builtin_popcount.

Patch looks good to me.  Thanks.

Florian
  
H.J. Lu July 1, 2017, 2:31 p.m. UTC | #2
On Sat, Jul 1, 2017 at 7:26 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 07/01/2017 04:12 PM, H.J. Lu wrote:
>> posix/sched_cpucount.c assumes that size of __cpu_mask == size of long,
>> which is incorrect for x32.  This patch uses __builtin_popcount, which
>> is availabe in GCC 4.9, in posix/sched_cpucount.c.
>>
>> Tested on i686, x86-64 and x32 with multi-arch disabled.
>>
>> 2017-07-01  Florian Weimer  <fweimer@redhat.com>
>>           H.J. Lu  <hongjiu.lu@intel.com>
>>
>>       [BZ #21696]
>>       * posix/sched_cpucount.c: Don't include <limits.h>.
>>       (__sched_cpucount): Use __builtin_popcount.
>
> Patch looks good to me.  Thanks.
>

Checked it in.

Thanks.
  

Patch

diff --git a/posix/sched_cpucount.c b/posix/sched_cpucount.c
index eaddf61..ab1ff49 100644
--- a/posix/sched_cpucount.c
+++ b/posix/sched_cpucount.c
@@ -15,7 +15,6 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <limits.h>
 #include <sched.h>
 
 
@@ -36,22 +35,16 @@  __sched_cpucount (size_t setsize, const cpu_set_t *setp)
       if (l == 0)
 	continue;
 
-# if LONG_BIT > 32
-      l = (l & 0x5555555555555555ul) + ((l >> 1) & 0x5555555555555555ul);
-      l = (l & 0x3333333333333333ul) + ((l >> 2) & 0x3333333333333333ul);
-      l = (l & 0x0f0f0f0f0f0f0f0ful) + ((l >> 4) & 0x0f0f0f0f0f0f0f0ful);
-      l = (l & 0x00ff00ff00ff00fful) + ((l >> 8) & 0x00ff00ff00ff00fful);
-      l = (l & 0x0000ffff0000fffful) + ((l >> 16) & 0x0000ffff0000fffful);
-      l = (l & 0x00000000fffffffful) + ((l >> 32) & 0x00000000fffffffful);
-# else
-      l = (l & 0x55555555ul) + ((l >> 1) & 0x55555555ul);
-      l = (l & 0x33333333ul) + ((l >> 2) & 0x33333333ul);
-      l = (l & 0x0f0f0f0ful) + ((l >> 4) & 0x0f0f0f0ful);
-      l = (l & 0x00ff00fful) + ((l >> 8) & 0x00ff00fful);
-      l = (l & 0x0000fffful) + ((l >> 16) & 0x0000fffful);
-# endif
-
-      s += l;
+      _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
     }