Skip to content

Commit

Permalink
[libc++][bit] Improves rotate functions.
Browse files Browse the repository at this point in the history
Investigating #70719 shows our implementation was different from the
Standard and could cause UB. Testing the codegen showed quite a bit of
assembly generated for these functions. The functions have been written
differently which allows Clang to optimize the code to use simple CPU
rotate instructions.

Fixes: #70719
  • Loading branch information
mordante committed Jul 8, 2024
1 parent f5b9e11 commit 58b547b
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 12 deletions.
37 changes: 25 additions & 12 deletions libcxx/include/__bit/rotate.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,37 @@

_LIBCPP_BEGIN_NAMESPACE_STD

// Writing two full functions for rotl and rotr makes it easier for the compiler
// to optimize the code. On x86 this function becomes the ROL instruction and
// the rotr function becomes the ROR instruction.
template <class _Tp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp __rotr(_Tp __t, int __cnt) _NOEXCEPT {
static_assert(__libcpp_is_unsigned_integer<_Tp>::value, "__rotr requires an unsigned integer type");
const unsigned int __dig = numeric_limits<_Tp>::digits;
if ((__cnt % __dig) == 0)
return __t;
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp __rotl(_Tp __x, int __s) _NOEXCEPT {
static_assert(__libcpp_is_unsigned_integer<_Tp>::value, "__rotl requires an unsigned integer type");
const unsigned int __N = numeric_limits<_Tp>::digits;
int __r = __s % __N;

if (__r == 0)
return __x;

if (__cnt < 0) {
__cnt *= -1;
return (__t << (__cnt % __dig)) | (__t >> (__dig - (__cnt % __dig))); // rotr with negative __cnt is similar to rotl
}
if (__r > 0)
return (__x << __r) | (__x >> (__N - __r));

return (__t >> (__cnt % __dig)) | (__t << (__dig - (__cnt % __dig)));
return (__x >> -__r) | (__x << (__N + __r));
}

template <class _Tp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp __rotl(_Tp __t, int __cnt) _NOEXCEPT {
return std::__rotr(__t, -__cnt);
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _Tp __rotr(_Tp __x, int __s) _NOEXCEPT {
static_assert(__libcpp_is_unsigned_integer<_Tp>::value, "__rotr requires an unsigned integer type");
const unsigned int __N = numeric_limits<_Tp>::digits;
int __r = __s % __N;

if (__r == 0)
return __x;

if (__r > 0)
return (__x >> __r) | (__x << (__N - __r));

return (__x << -__r) | (__x >> (__N + __r));
}

#if _LIBCPP_STD_VER >= 20
Expand Down
4 changes: 4 additions & 0 deletions libcxx/test/std/numerics/bit/bitops.rot/rotl.pass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ constexpr bool test()
assert(std::rotl(T(max - 1), 5) == T(max - 32));
assert(std::rotl(T(max - 1), 6) == T(max - 64));
assert(std::rotl(T(max - 1), 7) == T(max - 128));
assert(std::rotl(T(max - 1), std::numeric_limits<int>::max()) ==
std::rotl(T(max - 1), std::numeric_limits<int>::max() % std::numeric_limits<T>::digits));

assert(std::rotl(T(max - 1), -1) == T(max - highbit));
assert(std::rotl(T(max - 1), -2) == T(max - (highbit >> 1)));
Expand All @@ -49,6 +51,8 @@ constexpr bool test()
assert(std::rotl(T(max - 1), -5) == T(max - (highbit >> 4)));
assert(std::rotl(T(max - 1), -6) == T(max - (highbit >> 5)));
assert(std::rotl(T(max - 1), -7) == T(max - (highbit >> 6)));
assert(std::rotl(T(max - 1), std::numeric_limits<int>::min()) ==
std::rotl(T(max - 1), std::numeric_limits<int>::min() % std::numeric_limits<T>::digits));

assert(std::rotl(T(1), 0) == T(1));
assert(std::rotl(T(1), 1) == T(2));
Expand Down
4 changes: 4 additions & 0 deletions libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ constexpr bool test()
assert(std::rotr(T(max - 1), 5) == T(max - (highbit >> 4)));
assert(std::rotr(T(max - 1), 6) == T(max - (highbit >> 5)));
assert(std::rotr(T(max - 1), 7) == T(max - (highbit >> 6)));
assert(std::rotr(T(max - 1), std::numeric_limits<int>::max()) ==
std::rotr(T(max - 1), std::numeric_limits<int>::max() % std::numeric_limits<T>::digits));

assert(std::rotr(T(max - 1), -1) == T(max - 2));
assert(std::rotr(T(max - 1), -2) == T(max - 4));
Expand All @@ -49,6 +51,8 @@ constexpr bool test()
assert(std::rotr(T(max - 1), -5) == T(max - 32));
assert(std::rotr(T(max - 1), -6) == T(max - 64));
assert(std::rotr(T(max - 1), -7) == T(max - 128));
assert(std::rotr(T(max - 1), std::numeric_limits<int>::min()) ==
std::rotr(T(max - 1), std::numeric_limits<int>::min() % std::numeric_limits<T>::digits));

assert(std::rotr(T(128), 0) == T(128));
assert(std::rotr(T(128), 1) == T(64));
Expand Down

0 comments on commit 58b547b

Please sign in to comment.