[libc++] Replace __compressed_pair with [[no_unique_address]]
#76756
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
the
libc++
|
CC @cjdb |
philnik777
added
enhancement
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis significantly simplifies the code, improves compile times and improves the object layout of types using Patch is 138.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76756.diff 21 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index adff13e714cb64..e6d167140c99a0 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -168,6 +168,8 @@
// pointer from 16 to 8. This changes the output of std::string::max_size,
// which makes it ABI breaking
# define _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
+// Don't add padding from __compressed_pair
+# define _LIBCPP_ABI_NO_COMPRESSED_PAIR_PADDING
# elif _LIBCPP_ABI_VERSION == 1
# if !(defined(_LIBCPP_OBJECT_FORMAT_COFF) || defined(_LIBCPP_OBJECT_FORMAT_XCOFF))
// Enable compiling copies of now inline methods into the dylib to support
diff --git a/libcxx/include/__functional/function.h b/libcxx/include/__functional/function.h
index 6505bb5871739d..e8c2b5040a7ddc 100644
--- a/libcxx/include/__functional/function.h
+++ b/libcxx/include/__functional/function.h
@@ -138,45 +138,46 @@ class __default_alloc_func;
template <class _Fp, class _Ap, class _Rp, class... _ArgTypes>
class __alloc_func<_Fp, _Ap, _Rp(_ArgTypes...)> {
- __compressed_pair<_Fp, _Ap> __f_;
+ _LIBCPP_COMPRESSED_PAIR(_Fp, __func_, _Ap, __alloc_);
public:
typedef _LIBCPP_NODEBUG _Fp _Target;
typedef _LIBCPP_NODEBUG _Ap _Alloc;
- _LIBCPP_HIDE_FROM_ABI const _Target& __target() const { return __f_.first(); }
+ _LIBCPP_HIDE_FROM_ABI const _Target& __target() const { return __func_; }
// WIN32 APIs may define __allocator, so use __get_allocator instead.
- _LIBCPP_HIDE_FROM_ABI const _Alloc& __get_allocator() const { return __f_.second(); }
+ _LIBCPP_HIDE_FROM_ABI const _Alloc& __get_allocator() const { return __alloc_; }
- _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(_Target&& __f)
- : __f_(piecewise_construct, std::forward_as_tuple(std::move(__f)), std::forward_as_tuple()) {}
+ _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(_Target&& __f) : __func_(std::move(__f)), __alloc_() {}
- _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(const _Target& __f, const _Alloc& __a)
- : __f_(piecewise_construct, std::forward_as_tuple(__f), std::forward_as_tuple(__a)) {}
+ _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(const _Target& __f, const _Alloc& __a) : __func_(__f), __alloc_(__a) {}
_LIBCPP_HIDE_FROM_ABI explicit __alloc_func(const _Target& __f, _Alloc&& __a)
- : __f_(piecewise_construct, std::forward_as_tuple(__f), std::forward_as_tuple(std::move(__a))) {}
+ : __func_(__f), __alloc_(std::move(__a)) {}
_LIBCPP_HIDE_FROM_ABI explicit __alloc_func(_Target&& __f, _Alloc&& __a)
- : __f_(piecewise_construct, std::forward_as_tuple(std::move(__f)), std::forward_as_tuple(std::move(__a))) {}
+ : __func_(std::move(__f)), __alloc_(std::move(__a)) {}
_LIBCPP_HIDE_FROM_ABI _Rp operator()(_ArgTypes&&... __arg) {
typedef __invoke_void_return_wrapper<_Rp> _Invoker;
- return _Invoker::__call(__f_.first(), std::forward<_ArgTypes>(__arg)...);
+ return _Invoker::__call(__func_, std::forward<_ArgTypes>(__arg)...);
}
_LIBCPP_HIDE_FROM_ABI __alloc_func* __clone() const {
typedef allocator_traits<_Alloc> __alloc_traits;
typedef __rebind_alloc<__alloc_traits, __alloc_func> _AA;
- _AA __a(__f_.second());
+ _AA __a(__alloc_);
typedef __allocator_destructor<_AA> _Dp;
unique_ptr<__alloc_func, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
- ::new ((void*)__hold.get()) __alloc_func(__f_.first(), _Alloc(__a));
+ ::new ((void*)__hold.get()) __alloc_func(__func_, _Alloc(__a));
return __hold.release();
}
- _LIBCPP_HIDE_FROM_ABI void destroy() _NOEXCEPT { __f_.~__compressed_pair<_Target, _Alloc>(); }
+ _LIBCPP_HIDE_FROM_ABI void destroy() _NOEXCEPT {
+ __func_.~_Fp();
+ __alloc_.~_Alloc();
+ }
_LIBCPP_HIDE_FROM_ABI static void __destroy_and_delete(__alloc_func* __f) {
typedef allocator_traits<_Alloc> __alloc_traits;
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index 3cee48ef8538c6..87fd2984aabb9e 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -525,29 +525,29 @@ class __bucket_list_deallocator {
typedef allocator_traits<allocator_type> __alloc_traits;
typedef typename __alloc_traits::size_type size_type;
- __compressed_pair<size_type, allocator_type> __data_;
+ _LIBCPP_COMPRESSED_PAIR(size_type, __data_, allocator_type, __alloc_);
public:
typedef typename __alloc_traits::pointer pointer;
_LIBCPP_HIDE_FROM_ABI __bucket_list_deallocator() _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
- : __data_(0, __default_init_tag()) {}
+ : __data_(0) {}
_LIBCPP_HIDE_FROM_ABI __bucket_list_deallocator(const allocator_type& __a, size_type __size)
_NOEXCEPT_(is_nothrow_copy_constructible<allocator_type>::value)
- : __data_(__size, __a) {}
+ : __data_(__size), __alloc_(__a) {}
_LIBCPP_HIDE_FROM_ABI __bucket_list_deallocator(__bucket_list_deallocator&& __x)
_NOEXCEPT_(is_nothrow_move_constructible<allocator_type>::value)
- : __data_(std::move(__x.__data_)) {
+ : __data_(std::move(__x.__data_)), __alloc_(std::move(__x.__alloc_)) {
__x.size() = 0;
}
- _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __data_.first(); }
- _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __data_.first(); }
+ _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __data_; }
+ _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __data_; }
- _LIBCPP_HIDE_FROM_ABI allocator_type& __alloc() _NOEXCEPT { return __data_.second(); }
- _LIBCPP_HIDE_FROM_ABI const allocator_type& __alloc() const _NOEXCEPT { return __data_.second(); }
+ _LIBCPP_HIDE_FROM_ABI allocator_type& __alloc() _NOEXCEPT { return __alloc_; }
+ _LIBCPP_HIDE_FROM_ABI const allocator_type& __alloc() const _NOEXCEPT { return __alloc_; }
_LIBCPP_HIDE_FROM_ABI void operator()(pointer __p) _NOEXCEPT { __alloc_traits::deallocate(__alloc(), __p, size()); }
};
@@ -687,27 +687,27 @@ private:
// --- Member data begin ---
__bucket_list __bucket_list_;
- __compressed_pair<__first_node, __node_allocator> __p1_;
- __compressed_pair<size_type, hasher> __p2_;
- __compressed_pair<float, key_equal> __p3_;
+ _LIBCPP_COMPRESSED_PAIR(__first_node, __first_node_, __node_allocator, __node_alloc_);
+ _LIBCPP_COMPRESSED_PAIR(size_type, __size_, hasher, __hasher_);
+ _LIBCPP_COMPRESSED_PAIR(float, __max_load_factor_, key_equal, __key_eq_);
// --- Member data end ---
- _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __p2_.first(); }
+ _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __size_; }
public:
- _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __p2_.first(); }
+ _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __size_; }
- _LIBCPP_HIDE_FROM_ABI hasher& hash_function() _NOEXCEPT { return __p2_.second(); }
- _LIBCPP_HIDE_FROM_ABI const hasher& hash_function() const _NOEXCEPT { return __p2_.second(); }
+ _LIBCPP_HIDE_FROM_ABI hasher& hash_function() _NOEXCEPT { return __hasher_; }
+ _LIBCPP_HIDE_FROM_ABI const hasher& hash_function() const _NOEXCEPT { return __hasher_; }
- _LIBCPP_HIDE_FROM_ABI float& max_load_factor() _NOEXCEPT { return __p3_.first(); }
- _LIBCPP_HIDE_FROM_ABI float max_load_factor() const _NOEXCEPT { return __p3_.first(); }
+ _LIBCPP_HIDE_FROM_ABI float& max_load_factor() _NOEXCEPT { return __max_load_factor_; }
+ _LIBCPP_HIDE_FROM_ABI float max_load_factor() const _NOEXCEPT { return __max_load_factor_; }
- _LIBCPP_HIDE_FROM_ABI key_equal& key_eq() _NOEXCEPT { return __p3_.second(); }
- _LIBCPP_HIDE_FROM_ABI const key_equal& key_eq() const _NOEXCEPT { return __p3_.second(); }
+ _LIBCPP_HIDE_FROM_ABI key_equal& key_eq() _NOEXCEPT { return __key_eq_; }
+ _LIBCPP_HIDE_FROM_ABI const key_equal& key_eq() const _NOEXCEPT { return __key_eq_; }
- _LIBCPP_HIDE_FROM_ABI __node_allocator& __node_alloc() _NOEXCEPT { return __p1_.second(); }
- _LIBCPP_HIDE_FROM_ABI const __node_allocator& __node_alloc() const _NOEXCEPT { return __p1_.second(); }
+ _LIBCPP_HIDE_FROM_ABI __node_allocator& __node_alloc() _NOEXCEPT { return __node_alloc_; }
+ _LIBCPP_HIDE_FROM_ABI const __node_allocator& __node_alloc() const _NOEXCEPT { return __node_alloc_; }
public:
typedef __hash_iterator<__node_pointer> iterator;
@@ -991,26 +991,34 @@ inline __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table() _NOEXCEPT_(
is_nothrow_default_constructible<__bucket_list>::value&& is_nothrow_default_constructible<__first_node>::value&&
is_nothrow_default_constructible<__node_allocator>::value&& is_nothrow_default_constructible<hasher>::value&&
is_nothrow_default_constructible<key_equal>::value)
- : __p2_(0, __default_init_tag()), __p3_(1.0f, __default_init_tag()) {}
+ : __size_(0), __max_load_factor_(1.0f) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
inline __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const hasher& __hf, const key_equal& __eql)
- : __bucket_list_(nullptr, __bucket_list_deleter()), __p1_(), __p2_(0, __hf), __p3_(1.0f, __eql) {}
+ : __bucket_list_(nullptr, __bucket_list_deleter()),
+ __first_node_(),
+ __node_alloc_(),
+ __size_(0),
+ __hasher_(__hf),
+ __max_load_factor_(1.0f),
+ __key_eq_(__eql) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(
const hasher& __hf, const key_equal& __eql, const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, __hf),
- __p3_(1.0f, __eql) {}
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __hasher_(__hf),
+ __max_load_factor_(1.0f),
+ __key_eq_(__eql) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, __default_init_tag()),
- __p3_(1.0f, __default_init_tag()) {}
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __max_load_factor_(1.0f) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u)
@@ -1018,17 +1026,20 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u)
__bucket_list_deleter(allocator_traits<__pointer_allocator>::select_on_container_copy_construction(
__u.__bucket_list_.get_deleter().__alloc()),
0)),
- __p1_(__default_init_tag(),
- allocator_traits<__node_allocator>::select_on_container_copy_construction(__u.__node_alloc())),
- __p2_(0, __u.hash_function()),
- __p3_(__u.__p3_) {}
+ __node_alloc_(allocator_traits<__node_allocator>::select_on_container_copy_construction(__u.__node_alloc())),
+ __size_(0),
+ __hasher_(__u.hash_function()),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(__u.__key_eq_) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u, const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, __u.hash_function()),
- __p3_(__u.__p3_) {}
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __hasher_(__u.hash_function()),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(__u.__key_eq_) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u) _NOEXCEPT_(
@@ -1036,12 +1047,15 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u) _NOEX
is_nothrow_move_constructible<__node_allocator>::value&& is_nothrow_move_constructible<hasher>::value&&
is_nothrow_move_constructible<key_equal>::value)
: __bucket_list_(std::move(__u.__bucket_list_)),
- __p1_(std::move(__u.__p1_)),
- __p2_(std::move(__u.__p2_)),
- __p3_(std::move(__u.__p3_)) {
+ __first_node_(std::move(__u.__first_node_)),
+ __node_alloc_(std::move(__u.__node_alloc_)),
+ __size_(std::move(__u.__size_)),
+ __hasher_(std::move(__u.__hasher_)),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(std::move(__u.__key_eq_)) {
if (size() > 0) {
- __bucket_list_[std::__constrain_hash(__p1_.first().__next_->__hash(), bucket_count())] = __p1_.first().__ptr();
- __u.__p1_.first().__next_ = nullptr;
+ __bucket_list_[std::__constrain_hash(__first_node_.__next_->__hash(), bucket_count())] = __first_node_.__ptr();
+ __u.__first_node_.__next_ = nullptr;
__u.size() = 0;
}
}
@@ -1049,17 +1063,19 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u) _NOEX
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u, const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, std::move(__u.hash_function())),
- __p3_(std::move(__u.__p3_)) {
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __hasher_(std::move(__u.__hasher_)),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(std::move(__u.__key_eq_)) {
if (__a == allocator_type(__u.__node_alloc())) {
__bucket_list_.reset(__u.__bucket_list_.release());
__bucket_list_.get_deleter().size() = __u.__bucket_list_.get_deleter().size();
__u.__bucket_list_.get_deleter().size() = 0;
if (__u.size() > 0) {
- __p1_.first().__next_ = __u.__p1_.first().__next_;
- __u.__p1_.first().__next_ = nullptr;
- __bucket_list_[std::__constrain_hash(__p1_.first().__next_->__hash(), bucket_count())] = __p1_.first().__ptr();
+ __first_node_.__next_ = __u.__first_node_.__next_;
+ __u.__first_node_.__next_ = nullptr;
+ __bucket_list_[std::__constrain_hash(__first_node_.__next_->__hash(), bucket_count())] = __first_node_.__ptr();
size() = __u.size();
__u.size() = 0;
}
@@ -1073,7 +1089,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::~__hash_table() {
static_assert((is_copy_constructible<hasher>::value), "Hasher must be copy-constructible.");
#endif
- __deallocate_node(__p1_.first().__next_);
+ __deallocate_node(__first_node_.__next_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
@@ -1119,8 +1135,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__detach() _NOEXCEPT {
for (size_type __i = 0; __i < __bc; ++__i)
__bucket_list_[__i] = nullptr;
size() = 0;
- __next_pointer __cache = __p1_.first().__next_;
- __p1_.first().__next_ = nullptr;
+ __next_pointer __cache = __first_node_.__next_;
+ __first_node_.__next_ = nullptr;
return __cache;
}
@@ -1137,10 +1153,10 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__move_assign(__hash_table& __u,
hash_function() = std::move(__u.hash_function());
max_load_factor() = __u.max_load_factor();
key_eq() = std::move(__u.key_eq());
- __p1_.first().__next_ = __u.__p1_.first().__next_;
+ __first_node_.__next_ = __u.__first_node_.__next_;
if (size() > 0) {
- __bucket_list_[std::__constrain_hash(__p1_.first().__next_->__hash(), bucket_count())] = __p1_.first().__ptr();
- __u.__p1_.first().__next_ = nullptr;
+ __bucket_list_[std::__constrain_hash(__first_node_.__next_->__hash(), bucket_count())] = __first_node_.__ptr();
+ __u.__first_node_.__next_ = nullptr;
__u.size() = 0;
}
}
@@ -1257,7 +1273,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_multi(_InputIterator __f
template <class _Tp, class _Hash, class _Equal, class _Alloc>
inline typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
__hash_table<_Tp, _Hash, _Equal, _Alloc>::begin() _NOEXCEPT {
- return iterator(__p1_.first().__next_);
+ return iterator(__first_node_.__next_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
@@ -1269,7 +1285,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::end() _NOEXCEPT {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
inline typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::const_iterator
__hash_table<_Tp, _Hash, _Equal, _Alloc>::begin() const _NOEXCEPT {
- return const_iterator(__p1_.first().__next_);
+ return const_iterator(__first_node_.__next_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
@@ -1281,8 +1297,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::end() const _NOEXCEPT {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
void __hash_table<_Tp, _Hash, _Equal, _Alloc>::clear() _NOEXCEPT {
if (size() > 0) {
- __deallocate_node(__p1_.first().__next_);
- __p1_.first().__next_ = nullptr;
+ __deallocate_node(__first_node_.__next_);
+ __first_node_.__next_ = nullptr;
size_type __bc = bucket_count();
for (size_type __i = 0; __i < __bc; ++__i)
__bucket_list_[__i] = nullptr;
@@ -1334,7 +1350,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_unique_perform(__node_po
// insert_after __bucket_list_[__chash], or __first_node if bucket is null
__next_pointer __pn = __bucket_list_[__chash];
if (__pn == nullptr) {
- __pn = __p1_.first().__ptr();
+ __pn = __first_node_.__ptr();
__nd->__next_ = __pn->__next_;
__pn->__next_ = __nd->__ptr();
// fix up __bucket_list_
@@ -1414,7 +1430,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_multi_perform(
size_type __bc = bucket_count();
size_t __chash = std::__constrain_hash(__cp->__hash_, __bc);
if (__pn == nullptr) {
- __pn = __p1_.first().__ptr();
+ __pn = __first_node_.__ptr();
__cp->__next_ = __pn->__next_;
__pn->__next_ = __cp->__ptr();
// fix up __bucket_list_
@@ -1499,7 +1515,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__emplace_unique_key_args(_Key const&
// insert_after __bucket_list_[__chash], or __first_node if bucket is null
__next_pointer __pn = __bucket_list_[__chash];
if (__pn == nullptr) {
- __pn = __p1_.first().__ptr();
+ __pn = __first_node_.__ptr();
__h->__next_ = __pn->__next_;
__pn->__next_ = __h.get()->__ptr();
// fix up __bucket_list_
@@ -1677,7 +1693,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__do_rehash(size_type __nbc) {
if (__nbc > 0) {
for (size_type __i = 0; __i < __nbc; ++__i)
__bucket_list_[__i] = nullptr;
- __next_pointer __pp = __p1_.first().__ptr();
+ __next_pointer __pp = __first_node_.__ptr();
__next_pointer __cp = __pp->__next_;
if (__cp != nullptr) {
size_type __chash = std::__constrain_hash(__cp->__hash(), __nbc);
@@ -1854,7 +1870,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::remove(const_iterator __p) _NOEXCEPT {
// Fix up __bucket_list_
// if __pn is not in same bucket (before begin is not in same bucket) &&
// if __cn->__next_ is not in same bucket (nullptr is not in same bucket)
- if (__pn == __p1_.first().__ptr() || std::__constrain_hash(__pn->__hash(), __bc) != __chash) {
+ if (__pn == __first_node_.__ptr() || std::__constrain_hash(__pn->__hash(), __bc) != __chash) {
if (__cn->__next_ == nullptr || std::__constrain_hash(__cn->__next_->__hash(), __bc) !=...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
huixie90
reviewed
Jan 2, 2024
cjdb
reviewed
Jan 2, 2024
There was a problem hiding this comment.
Thanks for meticulously tracking all this stuff down! It's a welcome change that's long overdue.
My only concern is the one that @huixie90 raised. I wonder if we can get away with not keeping the padding, since it's a very niche situation. Once it catches up to trunk, I'll try building Chromium OS without any padding and see what breaks (that could be a few weeks though, so we should consider other ways to address this).
mordante
reviewed
Jan 3, 2024
| # define _LIBCPP_COMPRESSED_PAIR(T1, Name1, T2, Name2) \ | ||
| [[__gnu__::__aligned__(_LIBCPP_ALIGNOF(T2))]] _LIBCPP_NO_UNIQUE_ADDRESS T1 Name1; \ | ||
| _LIBCPP_COMPRESSED_PAIR_PADDING(T1, Name1##_padding_); \ | ||
| _LIBCPP_NO_UNIQUE_ADDRESS T2 Name2; \ |
There was a problem hiding this comment.
I have not looked in detail at the patch. Does this mean the _LIBCPP_NO_UNIQUE_ADDRESS affects the ABI?
There was a problem hiding this comment.
I think I'm missing something, but yes, [[no_unique_address]] affects the ABI.
There was a problem hiding this comment.
mainly that depending on the language version the ABI will be different.
There was a problem hiding this comment.
It won't be depending on the language version. [[no_unique_address]] always behaves the same.
There was a problem hiding this comment.
According to the __config _LIBCPP_NO_UNIQUE_ADDRESS can have different values. Since the attribute is C++20 this value can depend on the language version used, right? What am I missing?
There was a problem hiding this comment.
It can have in theory, but it doesn't. We can simplify that to
#if __has_cpp_attribute(msvc::no_unique_address)
# define _LIBCPP_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
#else
# define _LIBCPP_NO_UNIQUE_ADDRESS [[no_unique_address]]
#endifThere was a problem hiding this comment.
A great, do you want to create a small PR for that?
There was a problem hiding this comment.
I'll change it after the release branch.
|
@huixie90 I don't think explicitly adding padding bytes is a problem here. The affected classes are in just about no way trivial, so I don't think it matters how the padding bytes are generated. @cjdb it would be great to get some data whether we could drop the padding bytes, but my suspicion is that we can't, since |
ldionne
reviewed
Jan 18, 2024
...cxx/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
Show resolved
Hide resolved
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
8d3053c
to
20c2a36
Compare
hawkinsw
reviewed
Jan 24, 2024
ldionne
reviewed
Jan 25, 2024
libcxx/include/__memory/shared_ptr.h
Outdated
| return __elem; | ||
| } | ||
| _LIBCPP_HIDE_FROM_ABI _Alloc* __get_alloc() _NOEXCEPT { return std::addressof(__alloc_); } | ||
| _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_CFI _Tp* __get_elem() _NOEXCEPT { return std::addressof(__elem_); } |
There was a problem hiding this comment.
I'm pretty sure you can (and should) drop _LIBCPP_NO_CFI here since this was only useful when we did type punning. It's fine to do in a separate PR but let's drop it.
libcxx/include/vector
Outdated
| @@ -725,8 +726,7 @@ public: | |||
| private: | |||
| pointer __begin_ = nullptr; | |||
| pointer __end_ = nullptr; | |||
| __compressed_pair<pointer, allocator_type> __end_cap_ = | |||
| __compressed_pair<pointer, allocator_type>(nullptr, __default_init_tag()); | |||
| _LIBCPP_COMPRESSED_PAIR(pointer, __cap_, allocator_type, __alloc_); | |||
There was a problem hiding this comment.
You should be able to restore the = nullptr here and re-simplify the ctors after my suggestion to the macro.
libcxx/test/libcxx/containers/sequences/deque/abi.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/libcxx/containers/sequences/deque/abi.compile.pass.cpp
Outdated
Show resolved
Hide resolved
...cxx/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
Outdated
Show resolved
Hide resolved
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
20c2a36
to
2f30123
Compare
|
I just discussed with @philnik777 and I realized that this requires support for |
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
2f30123
to
4807741
Compare
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
3 times, most recently
from
5d0b99d
to
da991f8
Compare
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jun 24, 2024
This patch is in preparation for the `__compressed_pair` refactor in llvm#76756. This gets the formatter tests to at least pass. Currently in draft because there's still some cleanup to be done.
ldionne
requested changes
Aug 29, 2024
| __x.swap(__y); | ||
| } | ||
| # define _LIBCPP_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2) \ | ||
| [[__gnu__::__aligned__(_LIBCPP_ALIGNOF(T2))]] _LIBCPP_NO_UNIQUE_ADDRESS T1 Initializer1; \ |
There was a problem hiding this comment.
Suggested change
| [[__gnu__::__aligned__(_LIBCPP_ALIGNOF(T2))]] _LIBCPP_NO_UNIQUE_ADDRESS T1 Initializer1; \ | |
| _LIBCPP_NO_UNIQUE_ADDRESS __attribute__((__aligned__(_LIBCPP_ALIGNOF(T2)))) _LIBCPP_NO_UNIQUE_ADDRESS T1 Initializer1; \ |
This will solve some of the AppleClang / C++03 issues. Full patch:
diff --git a/libcxx/include/__memory/compressed_pair.h b/libcxx/include/__memory/compressed_pair.h
index 3729a7b05d48..13d35cd4ab6f 100644
--- a/libcxx/include/__memory/compressed_pair.h
+++ b/libcxx/include/__memory/compressed_pair.h
@@ -59,19 +59,19 @@ class __compressed_pair_padding {
};
# define _LIBCPP_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2) \
- [[__gnu__::__aligned__(_LIBCPP_ALIGNOF(T2))]] _LIBCPP_NO_UNIQUE_ADDRESS T1 Initializer1; \
- _LIBCPP_NO_UNIQUE_ADDRESS __compressed_pair_padding<T1> _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \
+ _LIBCPP_NO_UNIQUE_ADDRESS __attribute__((__aligned__(_LIBCPP_ALIGNOF(T2)))) T1 Initializer1; \
+ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T1> _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \
_LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2; \
- _LIBCPP_NO_UNIQUE_ADDRESS __compressed_pair_padding<T2> _LIBCPP_CONCAT3(__padding2_, __LINE__, _)
+ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T2> _LIBCPP_CONCAT3(__padding2_, __LINE__, _)
# define _LIBCPP_COMPRESSED_TRIPLE(T1, Initializer1, T2, Initializer2, T3, Initializer3) \
- [[__gnu__::__aligned__(_LIBCPP_ALIGNOF(T2)), __gnu__::__aligned__(_LIBCPP_ALIGNOF(T3))]] \
- _LIBCPP_NO_UNIQUE_ADDRESS T1 Initializer1; \
- _LIBCPP_NO_UNIQUE_ADDRESS __compressed_pair_padding<T1> _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \
+ _LIBCPP_NO_UNIQUE_ADDRESS \
+ __attribute__((__aligned__(_LIBCPP_ALIGNOF(T2)), __aligned__(_LIBCPP_ALIGNOF(T3)))) T1 Initializer1; \
+ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T1> _LIBCPP_CONCAT3(__padding1_, __LINE__, _); \
_LIBCPP_NO_UNIQUE_ADDRESS T2 Initializer2; \
- _LIBCPP_NO_UNIQUE_ADDRESS __compressed_pair_padding<T2> _LIBCPP_CONCAT3(__padding2_, __LINE__, _); \
+ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T2> _LIBCPP_CONCAT3(__padding2_, __LINE__, _); \
_LIBCPP_NO_UNIQUE_ADDRESS T3 Initializer3; \
- _LIBCPP_NO_UNIQUE_ADDRESS __compressed_pair_padding<T3> _LIBCPP_CONCAT3(__padding3_, __LINE__, _)
+ _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T3> _LIBCPP_CONCAT3(__padding3_, __LINE__, _)
#else
# define _LIBCPP_COMPRESSED_PAIR(T1, Name1, T2, Name2) \| @@ -54,9 +54,13 @@ struct _FirstPaddingByte<_Tp, true> { | |||
| // the use as an extension. | |||
| _LIBCPP_DIAGNOSTIC_PUSH | |||
There was a problem hiding this comment.
Patch for above:
diff --git a/libcxx/include/__type_traits/datasizeof.h b/libcxx/include/__type_traits/datasizeof.h
index 1b324e470d0b..657c7d917857 100644
--- a/libcxx/include/__type_traits/datasizeof.h
+++ b/libcxx/include/__type_traits/datasizeof.h
@@ -10,8 +10,6 @@
#define _LIBCPP___TYPE_TRAITS_DATASIZEOF_H
#include <__config>
-#include <__type_traits/is_class.h>
-#include <__type_traits/is_final.h>
#include <cstddef>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -30,25 +28,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD
template <class _Tp>
inline const size_t __datasizeof_v = __datasizeof(_Tp);
#else
-// NOLINTNEXTLINE(readability-redundant-preprocessor) This is https://llvm.org/PR64825
-# if __has_cpp_attribute(__no_unique_address__)
template <class _Tp>
struct _FirstPaddingByte {
- [[__no_unique_address__]] _Tp __v_;
+ _LIBCPP_NO_UNIQUE_ADDRESS _Tp __v_;
char __first_padding_byte_;
};
-# else
-template <class _Tp, bool = __libcpp_is_final<_Tp>::value || !is_class<_Tp>::value>
-struct _FirstPaddingByte : _Tp {
- char __first_padding_byte_;
-};
-
-template <class _Tp>
-struct _FirstPaddingByte<_Tp, true> {
- _Tp __v_;
- char __first_padding_byte_;
-};
-# endif // __has_cpp_attribute(__no_unique_address__)
// _FirstPaddingByte<> is sometimes non-standard layout. Using `offsetof` is UB in that case, but GCC and Clang allow
// the use as an extension.
@@ -61,7 +45,7 @@ _LIBCPP_DIAGNOSTIC_POP
template <class _Tp>
inline const size_t __datasizeof_v<_Tp&> = __datasizeof_v<_Tp>;
-#endif // __has_extension(datasizeof)
+#endif // __has_extension(datasizeof)
_LIBCPP_END_NAMESPACE_STD
And this needs additional tests. Specifically, we found out that an empty and final type has datasizeof == 1 when it should really have datasizeof == 0. This might be worth fixing on its own since this isn't related to this patch (in case e.g. this patch gets reverted).
philnik777
added a commit
that referenced
this pull request
Sep 5, 2024
These are used to ensure #76756 is correct.
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
3 times, most recently
from
9c31604
to
d09e626
Compare
ldionne
requested changes
Sep 5, 2024
libcxx/test/libcxx/containers/associative/unord.map/abi.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/libcxx/containers/associative/unord.set/abi.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/default.pass.cpp
Outdated
Show resolved
Hide resolved
...cxx/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
Outdated
Show resolved
Hide resolved
| @@ -143,7 +143,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr { | |||
| void>; | |||
|
|
|||
| private: | |||
| __compressed_pair<pointer, deleter_type> __ptr_; | |||
| _LIBCPP_COMPRESSED_PAIR(pointer, __ptr_, deleter_type, __deleter_); | |||
There was a problem hiding this comment.
Since we know the before and after layout, we could issue a diagnostic (static_assert) statically when we have an ABI break. We would provide an opt-out for users to disable the error if they don't care about the breakage. We would remove that diagnostic in LLVM 21, giving one release for users to figure out what to do if they're affected.
Can you look into this and we can run it through a large code base to get a bit more information about the potential unintended impact of this change.
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Sep 12, 2024
These are used to ensure llvm#76756 is correct.
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
2 times, most recently
from
c8ed751
to
509ab33
Compare
|
CC @llvm/libcxx-vendors for awareness since this affects the ABI in minor ways. |
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
509ab33
to
8a608fc
Compare
ldionne
approved these changes
Sep 12, 2024
There was a problem hiding this comment.
LGTM. Let's land this once the CI is green (and after checking whether we can split the datasizeof change out).
Once you land this, please coordinate with LLDB for the data formatters change. Thanks for pushing this through, this is an amazing improvement that required some of the most advanced technical skills I've seen. This is impressive.
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
8a608fc
to
a781989
Compare
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
a781989
to
b8a1e86
Compare
|
@Michael137 This time I'll actually land it :D. Would landing the patches tomorrow ~9 a.m. UTC work for you? |
Works! I'll keep an eye out |
Michael137
added a commit
that referenced
this pull request
Sep 16, 2024
This patch is in preparation for the `__compressed_pair` refactor in #76756. This is mostly reviewable now. With the new layout we no longer need to unwrap the `__compressed_pair`. Instead, we just need to look for child members. E.g., to get to the underlying pointer of `std::unique_ptr` we no longer do, ``` GetFirstValueOfCXXCompressedPair(GetChildMemberWithName("__ptr_")) ``` but instead do ``` GetChildMemberWithName("__ptr_") ``` We need to be slightly careful because previously the `__compressed_pair` had a member called `__value_`, whereas now `__value_` might be a member of the class that used to hold the `__compressed_pair`. So before unwrapping the pair, we added checks for `isOldCompressedLayout` (not sure yet whether folding this check into `GetFirstValueOfCXXCompressedPair` is better).
philnik777
added a commit
to philnik777/llvm-project
that referenced
this pull request
Sep 17, 2024
philnik777
added a commit
to philnik777/llvm-project
that referenced
this pull request
Sep 17, 2024
philnik777
added a commit
to philnik777/llvm-project
that referenced
this pull request
Sep 17, 2024
|
This change broke compiling Qt. The following piece of code fails: https://github.com/qt/qtbase/blob/v6.8.0-beta4/src/corelib/text/qregularexpression.cpp#L952-L962 The change can be reproduced with a small isolated snippet: #include <memory>
#define Q_CONSTINIT [[clang::require_constant_initialization]]
typedef struct pcre2_jit_stack_16 pcre2_jit_stack_16;
void pcre2_jit_stack_free_16(pcre2_jit_stack_16 *stack);
struct PcreJitStackFree {
void operator()(pcre2_jit_stack_16 *stack) {
if (stack)
pcre2_jit_stack_free_16(stack);
}
};
Q_CONSTINIT std::unique_ptr<pcre2_jit_stack_16, PcreJitStackFree> jitStacks;Compiling this produces the following errors: This snippet does compile successfully in C++20 mode, but not in C++17 mode - while it did build successfully before. |
|
@mstorsjo it looks like the problem is just that we don't initialize the padding: https://godbolt.org/z/ErETWTdoc. Could you check that this fixes your problem too? Initializing the padding bytes seems fairly low cost, especially since the common cases don't actually have any padding bytes ( |
Thanks for the quick reply! So essentially, I'd apply the following change: diff --git a/libcxx/include/__memory/compressed_pair.h b/libcxx/include/__memory/compressed_pair.h
index 629e3ad8848f..12404953a715 100644
--- a/libcxx/include/__memory/compressed_pair.h
+++ b/libcxx/include/__memory/compressed_pair.h
@@ -56,7 +56,7 @@ template <class _ToPad>
class __compressed_pair_padding {
char __padding_[((is_empty<_ToPad>::value && !__libcpp_is_final<_ToPad>::value) || is_reference<_ToPad>::value)
? 0
- : sizeof(_ToPad) - __datasizeof_v<_ToPad>];
+ : sizeof(_ToPad) - __datasizeof_v<_ToPad>]{};
};
# define _LIBCPP_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2) \That does seem to fix building Qt, thanks! (I guess this can be considered a gap in our testsuite coverage, that we should patch at some point as well?) However it does break including these headers in C++98/03 mode: |
philnik777
added a commit
to philnik777/llvm-project
that referenced
this pull request
Sep 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This significantly simplifies the code, improves compile times and improves the object layout of types using
__compressed_pairin the unstable ABI. The only downside is that this is extremely ABI sensitive and pedantically breaks the ABI for empty final types, since the address of the subobject may change. The ABI of the whole object should not be affected.Fixes #91266
Fixes #93069