Skip to content

[libc++][atomic_ref] Use __atomic_fetch_{add,sub} builtins on floating-points whenever possible #135685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions libcxx/include/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ set(files
__atomic/check_memory_order.h
__atomic/contention_t.h
__atomic/fence.h
__atomic/floating_point_helper.h
__atomic/is_always_lock_free.h
__atomic/kill_dependency.h
__atomic/memory_order.h
Expand Down
29 changes: 3 additions & 26 deletions libcxx/include/__atomic/atomic.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include <__atomic/atomic_sync.h>
#include <__atomic/check_memory_order.h>
#include <__atomic/floating_point_helper.h>
#include <__atomic/is_always_lock_free.h>
#include <__atomic/memory_order.h>
#include <__atomic/support.h>
Expand Down Expand Up @@ -332,41 +333,17 @@ template <class _Tp>
requires is_floating_point_v<_Tp>
struct atomic<_Tp> : __atomic_base<_Tp> {
private:
_LIBCPP_HIDE_FROM_ABI static constexpr bool __is_fp80_long_double() {
// Only x87-fp80 long double has 64-bit mantissa
return __LDBL_MANT_DIG__ == 64 && std::is_same_v<_Tp, long double>;
}

_LIBCPP_HIDE_FROM_ABI static constexpr bool __has_rmw_builtin() {
# ifndef _LIBCPP_COMPILER_CLANG_BASED
return false;
# else
// The builtin __cxx_atomic_fetch_add errors during compilation for
// long double on platforms with fp80 format.
// For more details, see
// lib/Sema/SemaChecking.cpp function IsAllowedValueType
// LLVM Parser does not allow atomicrmw with x86_fp80 type.
// if (ValType->isSpecificBuiltinType(BuiltinType::LongDouble) &&
// &Context.getTargetInfo().getLongDoubleFormat() ==
// &llvm::APFloat::x87DoubleExtended())
// For more info
// https://github.com/llvm/llvm-project/issues/68602
// https://reviews.llvm.org/D53965
return !__is_fp80_long_double();
# endif
}

template <class _This, class _Operation, class _BuiltinOp>
_LIBCPP_HIDE_FROM_ABI static _Tp
__rmw_op(_This&& __self, _Tp __operand, memory_order __m, _Operation __operation, _BuiltinOp __builtin_op) {
if constexpr (__has_rmw_builtin()) {
if constexpr (std::__has_rmw_builtin<_Tp>()) {
return __builtin_op(std::addressof(std::forward<_This>(__self).__a_), __operand, __m);
} else {
_Tp __old = __self.load(memory_order_relaxed);
_Tp __new = __operation(__old, __operand);
while (!__self.compare_exchange_weak(__old, __new, __m, memory_order_relaxed)) {
# ifdef _LIBCPP_COMPILER_CLANG_BASED
if constexpr (__is_fp80_long_double()) {
if constexpr (std::__is_fp80_long_double<_Tp>()) {
// https://github.com/llvm/llvm-project/issues/47978
// clang bug: __old is not updated on failure for atomic<long double>::compare_exchange_weak
// Note __old = __self.load(memory_order_relaxed) will not work
Expand Down
29 changes: 19 additions & 10 deletions libcxx/include/__atomic/atomic_ref.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <__assert>
#include <__atomic/atomic_sync.h>
#include <__atomic/check_memory_order.h>
#include <__atomic/floating_point_helper.h>
#include <__atomic/memory_order.h>
#include <__atomic/to_gcc_order.h>
#include <__concepts/arithmetic.h>
Expand Down Expand Up @@ -322,20 +323,28 @@ struct atomic_ref<_Tp> : public __atomic_ref_base<_Tp> {
atomic_ref& operator=(const atomic_ref&) = delete;

_LIBCPP_HIDE_FROM_ABI _Tp fetch_add(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
_Tp __old = this->load(memory_order_relaxed);
_Tp __new = __old + __arg;
while (!this->compare_exchange_weak(__old, __new, __order, memory_order_relaxed)) {
__new = __old + __arg;
if constexpr (std::__has_rmw_builtin<_Tp>()) {
return __atomic_fetch_add(this->__ptr_, __arg, std::__to_gcc_order(__order));
} else {
_Tp __old = this->load(memory_order_relaxed);
_Tp __new = __old + __arg;
while (!this->compare_exchange_weak(__old, __new, __order, memory_order_relaxed)) {
__new = __old + __arg;
}
return __old;
}
return __old;
}
_LIBCPP_HIDE_FROM_ABI _Tp fetch_sub(_Tp __arg, memory_order __order = memory_order_seq_cst) const noexcept {
_Tp __old = this->load(memory_order_relaxed);
_Tp __new = __old - __arg;
while (!this->compare_exchange_weak(__old, __new, __order, memory_order_relaxed)) {
__new = __old - __arg;
if constexpr (std::__has_rmw_builtin<_Tp>()) {
return __atomic_fetch_sub(this->__ptr_, __arg, std::__to_gcc_order(__order));
} else {
_Tp __old = this->load(memory_order_relaxed);
_Tp __new = __old - __arg;
while (!this->compare_exchange_weak(__old, __new, __order, memory_order_relaxed)) {
__new = __old - __arg;
}
return __old;
}
return __old;
}

_LIBCPP_HIDE_FROM_ABI _Tp operator+=(_Tp __arg) const noexcept { return fetch_add(__arg) + __arg; }
Expand Down
55 changes: 55 additions & 0 deletions libcxx/include/__atomic/floating_point_helper.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef _LIBCPP___ATOMIC_FLOATING_POINT_HELPER_H
#define _LIBCPP___ATOMIC_FLOATING_POINT_HELPER_H

#include <__config>
#include <__type_traits/is_floating_point.h>
#include <__type_traits/is_same.h>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
#endif

_LIBCPP_BEGIN_NAMESPACE_STD

#if _LIBCPP_STD_VER >= 20

template <class _Tp>
_LIBCPP_HIDE_FROM_ABI constexpr bool __is_fp80_long_double() {
// Only x87-fp80 long double has 64-bit mantissa
return __LDBL_MANT_DIG__ == 64 && std::is_same_v<_Tp, long double>;
}

template <class _Tp>
_LIBCPP_HIDE_FROM_ABI constexpr bool __has_rmw_builtin() {
static_assert(std::is_floating_point_v<_Tp>);
# ifndef _LIBCPP_COMPILER_CLANG_BASED
return false;
# else
// The builtin __cxx_atomic_fetch_add errors during compilation for
// long double on platforms with fp80 format.
// For more details, see
// lib/Sema/SemaChecking.cpp function IsAllowedValueType
// LLVM Parser does not allow atomicrmw with x86_fp80 type.
// if (ValType->isSpecificBuiltinType(BuiltinType::LongDouble) &&
// &Context.getTargetInfo().getLongDoubleFormat() ==
// &llvm::APFloat::x87DoubleExtended())
// For more info
// https://github.com/llvm/llvm-project/issues/68602
// https://reviews.llvm.org/D53965
return !std::__is_fp80_long_double<_Tp>();
# endif
}

#endif

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP___ATOMIC_FLOATING_POINT_HELPER_H
1 change: 1 addition & 0 deletions libcxx/include/module.modulemap.in
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,7 @@ module std [system] {
module check_memory_order { header "__atomic/check_memory_order.h" }
module contention_t { header "__atomic/contention_t.h" }
module fence { header "__atomic/fence.h" }
module floating_point_helper { header "__atomic/floating_point_helper.h" }
module is_always_lock_free { header "__atomic/is_always_lock_free.h" }
module kill_dependency { header "__atomic/kill_dependency.h" }
module memory_order { header "__atomic/memory_order.h" }
Expand Down
1 change: 1 addition & 0 deletions libcxx/test/std/atomics/atomics.ref/fetch_add.pass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

// UNSUPPORTED: c++03, c++11, c++14, c++17
// XFAIL: !has-64-bit-atomics
// XFAIL: target={{x86_64-.*}} && tsan
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from

// https://github.com/llvm/llvm-project/issues/72893
// XFAIL: target={{x86_64-.*}} && tsan

Note that the #72893 issue referenced in the comment seems to indicate that only long double was problematic with ThreadSanitizer but it is not accurate because the instantiation of the tests are commented out for long double

// TODO https://github.com/llvm/llvm-project/issues/47978
// test<long double>();

Per a conversation on Discord with @huixie90, I propose to not hold up this PR and just use the same XFAIL annotation that we have in the std::atomic<Floating>::fetch_{add,sub} tests.


// integral-type fetch_add(integral-type, memory_order = memory_order::seq_cst) const noexcept;
// floating-point-type fetch_add(floating-point-type, memory_order = memory_order::seq_cst) const noexcept;
Expand Down
1 change: 1 addition & 0 deletions libcxx/test/std/atomics/atomics.ref/fetch_sub.pass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

// UNSUPPORTED: c++03, c++11, c++14, c++17
// XFAIL: !has-64-bit-atomics
// XFAIL: target={{x86_64-.*}} && tsan

// integral-type fetch_sub(integral-type, memory_order = memory_order::seq_cst) const noexcept;
// floating-point-type fetch_sub(floating-point-type, memory_order = memory_order::seq_cst) const noexcept;
Expand Down
1 change: 1 addition & 0 deletions llvm/utils/gn/secondary/libcxx/include/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ if (current_toolchain == default_toolchain) {
"__atomic/check_memory_order.h",
"__atomic/contention_t.h",
"__atomic/fence.h",
"__atomic/floating_point_helper.h",
"__atomic/is_always_lock_free.h",
"__atomic/kill_dependency.h",
"__atomic/memory_order.h",
Expand Down
Loading