Re: [PATCH] P0556R3 Integral power-of-2 operations, P0553R2 Bit operations

[Jonathan Wakely <jwakely at redhat dot com>]

On 03/07/18 23:23 +0100, Jonathan Wakely wrote:
> On 03/07/18 23:40 +0200, Jakub Jelinek wrote:
> > On Tue, Jul 03, 2018 at 10:02:47PM +0100, Jonathan Wakely wrote:
> > > +#ifndef _GLIBCXX_BIT
> > > +#define _GLIBCXX_BIT 1
> > > +
> > > +#pragma GCC system_header
> > > +
> > > +#if __cplusplus >= 201402L
> > > +
> > > +#include <type_traits>
> > > +#include <limits>
> > > +
> > > +namespace std _GLIBCXX_VISIBILITY(default)
> > > +{
> > > +_GLIBCXX_BEGIN_NAMESPACE_VERSION
> > > +
> > > +  template<typename _Tp>
> > > +    constexpr _Tp
> > > +    __rotl(_Tp __x, unsigned int __s) noexcept
> > > +    {
> > > +      constexpr auto _Nd = numeric_limits<_Tp>::digits;
> > > +      const unsigned __sN = __s % _Nd;
> > > +      if (__sN)
> > > +        return (__x << __sN) | (__x >> (_Nd - __sN));
> >
> > Wouldn't it be better to use some branchless pattern that
> > GCC can also optimize well, like:
> >     return (__x << __sN) | (__x >> ((-_sN) & (_Nd - 1)));
> > (iff _Nd is always power of two),
>
> _Nd is 20 for one of the INT_N types on msp340, but we could have a
> special case for the rare integer types with unusual sizes.
>
> > or perhaps
> >     return (__x << __sN) | (__x >> ((-_sN) % _Nd));
> > which is going to be folded into the above one for power of two constants?
>
> That looks good.

Committed as attached (with a test I forgot to add for popcount).


commit 796e90be02c691bc57c89172940aa047caa199ab
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Jul 4 00:07:43 2018 +0100

    Optimize std::rotl and std::rotr, add test for std::popcount
    
            * include/std/bit (__rotl, __rotr): Avoid branch.
            (_If_is_unsigned_integer): Use remove_cv_t.
            * testsuite/26_numerics/bit/bitops.count/popcount.cc: New.

diff --git a/libstdc++-v3/include/std/bit b/libstdc++-v3/include/std/bit
index 76aa0957b56..ace88954030 100644
--- a/libstdc++-v3/include/std/bit
+++ b/libstdc++-v3/include/std/bit
@@ -46,9 +46,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       constexpr auto _Nd = numeric_limits<_Tp>::digits;
       const unsigned __sN = __s % _Nd;
-      if (__sN)
-        return (__x << __sN) | (__x >> (_Nd - __sN));
-      return __x;
+      return (__x << __sN) | (__x >> ((-__sN) % _Nd));
     }
 
   template<typename _Tp>
@@ -57,9 +55,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       constexpr auto _Nd = numeric_limits<_Tp>::digits;
       const unsigned __sN = __s % _Nd;
-      if (__sN)
-        return (__x >> __sN) | (__x << (_Nd - __sN));
-      return __x;
+      return (__x >> __sN) | (__x << ((-__sN) % _Nd));
     }
 
   template<typename _Tp>
@@ -237,7 +233,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   template<typename _Tp, typename _Up = _Tp>
     using _If_is_unsigned_integer
-      = typename _If_is_unsigned_integer_type<_Tp, _Up>::type;
+      = typename _If_is_unsigned_integer_type<remove_cv_t<_Tp>, _Up>::type;
 
 #if ! __STRICT_ANSI__
   // [bitops.rot], rotating
diff --git a/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/popcount.cc b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/popcount.cc
new file mode 100644
index 00000000000..2982cb19bbe
--- /dev/null
+++ b/libstdc++-v3/testsuite/26_numerics/bit/bitops.count/popcount.cc
@@ -0,0 +1,108 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This 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 General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+
+// { dg-options "-std=gnu++2a" }
+// { dg-do compile { target c++2a } }
+
+#include <bit>
+
+template<typename UInt>
+constexpr auto
+test(UInt x)
+-> decltype(std::popcount(x))
+{
+  static_assert( noexcept(std::popcount(x)) );
+
+  constexpr unsigned digits = std::numeric_limits<UInt>::digits;
+
+  static_assert( std::popcount((UInt)0) == 0 );
+  static_assert( std::popcount((UInt)-1) == digits );
+  static_assert( std::popcount((UInt)-2) == digits - 1 );
+  static_assert( std::popcount((UInt)127) == 7 );
+
+  static_assert( std::popcount((UInt)1) == 1 );
+  static_assert( std::popcount((UInt)2) == 1 );
+  static_assert( std::popcount((UInt)0x70) == 3 );
+
+  if constexpr (std::numeric_limits<UInt>::digits > 8)
+  {
+    static_assert( std::popcount((UInt)(0x101)) == 2 );
+    static_assert( std::popcount((UInt)(0xfff)) == 12 );
+  }
+
+  if constexpr (std::numeric_limits<UInt>::digits > 64)
+  {
+    static_assert( std::popcount((UInt)0xffffffffffffffff) == 64 );
+    static_assert( std::popcount(0x5555555555555555 | ((UInt)1 << 64)) == 33 );
+    static_assert( std::popcount(0x5555555555555555 | ((UInt)3 << 64)) == 34 );
+  }
+
+  return true;
+}
+
+static_assert( test( (unsigned char)0 ) );
+static_assert( test( (unsigned short)0 ) );
+static_assert( test( (unsigned int)0 ) );
+static_assert( test( (unsigned long)0 ) );
+static_assert( test( (unsigned long long)0 ) );
+
+// std::popcount(T) shall not participate in overload resolution
+// unless T is an unsigned integer type.
+struct X { constexpr bool did_not_match() { return true; } };
+constexpr X test(...) { return X{}; }
+static_assert( test( (bool)0 ).did_not_match() );
+static_assert( test( (char)0 ).did_not_match() );
+static_assert( test( (int)0 ).did_not_match() );
+static_assert( test( (char16_t)0 ).did_not_match() );
+static_assert( test( (float)0 ).did_not_match() );
+static_assert( test( (void*)0 ).did_not_match() );
+static_assert( test( X{} ).did_not_match() );
+enum E : unsigned { e };
+static_assert( test( e ).did_not_match() );
+
+#ifndef __STRICT_ANSI__
+#include <cstddef>
+static_assert( std::popcount(std::byte{0x00}) == 0 );
+static_assert( std::popcount(std::byte{0x01}) == 1 );
+static_assert( std::popcount(std::byte{0x02}) == 1 );
+static_assert( std::popcount(std::byte{0x03}) == 2 );
+static_assert( std::popcount(std::byte{0x30}) == 2 );
+static_assert( std::popcount(std::byte{0x40}) == 1 );
+static_assert( std::popcount(std::byte{0x41}) == 2 );
+static_assert( std::popcount(std::byte{0xff}) == 8 );
+#else
+static_assert( test( (std::byte)0 ).did_not_match() );
+#endif
+
+#if !defined(__STRICT_ANSI__) && defined _GLIBCXX_USE_INT128
+static_assert( test( (unsigned __int128)0 ) );
+static_assert( test( (__int128)0 ).did_not_match() );
+#endif
+#if defined(__GLIBCXX_TYPE_INT_N_0)
+static_assert( test( (unsigned __GLIBCXX_TYPE_INT_N_0)0 ) );
+static_assert( test( (__GLIBCXX_TYPE_INT_N_0)0 ).did_not_match() );
+#endif
+#if defined(__GLIBCXX_TYPE_INT_N_1)
+static_assert( test( (unsigned __GLIBCXX_TYPE_INT_N_1)0 ) );
+static_assert( test( (__GLIBCXX_TYPE_INT_N_1)0 ).did_not_match() );
+#endif
+#if defined(__GLIBCXX_TYPE_INT_N_2)
+static_assert( test( (unsigned __GLIBCXX_TYPE_INT_N_2)0 ) );
+static_assert( test( (__GLIBCXX_TYPE_INT_N_2)0 ).did_not_match() );
+#endif