[libc++] Remove some unnecessary functions from __vector_layout

[philnik777]

This removes functions which produce identical IR after the first InstCombine pass after inlining compared to their replacements.

[llvmorg-github-actions]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This removes functions which produce identical IR after the first InstCombine pass after inlining compared to their replacements.

---

Full diff: https://github.com/llvm/llvm-project/pull/207152.diff

2 Files Affected:

• (modified) libcxx/include/__vector/layout.h (-46)
• (modified) libcxx/include/__vector/vector.h (+8-8)

diff --git a/libcxx/include/__vector/layout.h b/libcxx/include/__vector/layout.h
index 65d11aa238df0..6be670381b244 100644
--- a/libcxx/include/__vector/layout.h
+++ b/libcxx/include/__vector/layout.h
@@ -179,9 +179,6 @@ class __vector_layout {
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type
   __remaining_capacity() const _NOEXCEPT;
 
-  /// Determines if a reallocation is necessary.
-  [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool __is_full() const _NOEXCEPT;
-
   /// Sets the member pointing to the first element in the vector to `__new_begin`, the member used
   /// to obtain the vector's bound to the equivalent of `__new_size`, and the member that represents
   /// the vector's capacity to the equivalent to `__new_capacity`.
@@ -202,9 +199,6 @@ class __vector_layout {
 
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type __size() const _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type __capacity() const _NOEXCEPT;
-  [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool __empty() const _NOEXCEPT;
-  [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI _Tp& __back() _NOEXCEPT;
-  [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI _Tp const& __back() const _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI pointer __end_ptr() _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_pointer __end_ptr() const _NOEXCEPT;
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI pointer __capacity_ptr() _NOEXCEPT;
@@ -252,11 +246,6 @@ __vector_layout<_Tp, _Alloc>::__remaining_capacity() const _NOEXCEPT {
   return __capacity_ - __size_;
 }
 
-template <class _Tp, class _Alloc>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 bool __vector_layout<_Tp, _Alloc>::__is_full() const _NOEXCEPT {
-  return __size_ == __capacity_;
-}
-
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void __vector_layout<_Tp, _Alloc>::__set_layout(
     pointer __new_begin, size_type __new_size, size_type __new_capacity) _NOEXCEPT {
@@ -324,21 +313,6 @@ __vector_layout<_Tp, _Alloc>::__capacity() const _NOEXCEPT {
   return __capacity_;
 }
 
-template <class _Tp, class _Alloc>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 bool __vector_layout<_Tp, _Alloc>::__empty() const _NOEXCEPT {
-  return __size_ == 0;
-}
-
-template <class _Tp, class _Alloc>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp& __vector_layout<_Tp, _Alloc>::__back() _NOEXCEPT {
-  return __begin_[__size_ - 1];
-}
-
-template <class _Tp, class _Alloc>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp const& __vector_layout<_Tp, _Alloc>::__back() const _NOEXCEPT {
-  return __begin_[__size_ - 1];
-}
-
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename __vector_layout<_Tp, _Alloc>::pointer
 __vector_layout<_Tp, _Alloc>::__end_ptr() _NOEXCEPT {
@@ -384,11 +358,6 @@ __vector_layout<_Tp, _Alloc>::__remaining_capacity() const _NOEXCEPT {
   return __capacity_ - __end_;
 }
 
-template <class _Tp, class _Alloc>
-[[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __vector_layout<_Tp, _Alloc>::__is_full() const _NOEXCEPT {
-  return __end_ == __capacity_;
-}
-
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void __vector_layout<_Tp, _Alloc>::__set_layout(
     pointer __new_begin, size_type __new_size, size_type __new_capacity) _NOEXCEPT {
@@ -456,21 +425,6 @@ __vector_layout<_Tp, _Alloc>::__capacity() const _NOEXCEPT {
   return static_cast<size_type>(__capacity_ - __begin_);
 }
 
-template <class _Tp, class _Alloc>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 bool __vector_layout<_Tp, _Alloc>::__empty() const _NOEXCEPT {
-  return __begin_ == __end_;
-}
-
-template <class _Tp, class _Alloc>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp& __vector_layout<_Tp, _Alloc>::__back() _NOEXCEPT {
-  return __end_[-1];
-}
-
-template <class _Tp, class _Alloc>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp const& __vector_layout<_Tp, _Alloc>::__back() const _NOEXCEPT {
-  return __end_[-1];
-}
-
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename __vector_layout<_Tp, _Alloc>::pointer
 __vector_layout<_Tp, _Alloc>::__end_ptr() _NOEXCEPT {
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 2b9508ecafeac..ae55dfbd877c1 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -400,7 +400,7 @@ class vector {
     return __layout_.__capacity();
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI bool empty() const _NOEXCEPT {
-    return __layout_.__empty();
+    return size() == 0;
   }
 
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type max_size() const _NOEXCEPT {
@@ -442,11 +442,11 @@ class vector {
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference back() _NOEXCEPT {
     _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "back() called on an empty vector");
-    return __layout_.__back();
+    return end()[-1];
   }
   [[__nodiscard__]] _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI const_reference back() const _NOEXCEPT {
     _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(!empty(), "back() called on an empty vector");
-    return __layout_.__back();
+    return end()[-1];
   }
 
   //
@@ -1080,7 +1080,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI typename vector<_Tp, _Alloc>
 vector<_Tp, _Alloc>::emplace_back(_Args&&... __args) {
   pointer __end = __layout_.__end_ptr();
   std::__if_likely_else(
-      !__layout_.__is_full(),
+      size() != capacity(),
       [&] {
         __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
         ++__end;
@@ -1134,7 +1134,7 @@ template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
 vector<_Tp, _Allocator>::insert(const_iterator __position, const_reference __x) {
   pointer __p = this->__layout_.__begin_ptr() + (__position - begin());
-  if (!__layout_.__is_full()) {
+  if (size() != capacity()) {
     pointer __end = __layout_.__end_ptr();
     if (__p == __end) {
       __emplace_back_assume_capacity(__x);
@@ -1157,7 +1157,7 @@ template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
 vector<_Tp, _Allocator>::insert(const_iterator __position, value_type&& __x) {
   pointer __p = this->__layout_.__begin_ptr() + (__position - begin());
-  if (!__layout_.__is_full()) {
+  if (size() != capacity()) {
     pointer __end = __layout_.__end_ptr();
     if (__p == __end) {
       __emplace_back_assume_capacity(std::move(__x));
@@ -1178,7 +1178,7 @@ template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::iterator
 vector<_Tp, _Allocator>::emplace(const_iterator __position, _Args&&... __args) {
   pointer __p = this->__layout_.__begin_ptr() + (__position - begin());
-  if (!__layout_.__is_full()) {
+  if (size() != capacity()) {
     pointer __end = __layout_.__end_ptr();
     if (__p == __end) {
       __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
@@ -1232,7 +1232,7 @@ vector<_Tp, _Allocator>::__insert_with_sentinel(const_iterator __position, _Inpu
   difference_type __off = __position - begin();
   pointer __p           = this->__layout_.__begin_ptr() + __off;
   pointer __old_last    = __layout_.__end_ptr();
-  for (; !__layout_.__is_full() && __first != __last; ++__first)
+  for (; size() != capacity() && __first != __last; ++__first)
     __emplace_back_assume_capacity(*__first);
 
   if (__first == __last)

[ldionne]

I tend to like that: minimizing the number of functions in the layout classes is a good thing. I had been under the impression that this actually made a difference codegen wise, but I guess it makes sense that it doesn't.

I'd like to look again with green CI and to chat about this during a live review to make sure there's no chance of this regressing perf.

[ldionne]

I think this makes sense. As discussed, you verified these didn't make any difference codegen wise, so I think we could simply not have introduced them in the first place, to keep the layout API minimal.

[nico]

size() has to do arithmetic on the pointers, which requires complete types. The LHS, and vector before _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED, didn't require complete types to call empty().

Compete types are only needed if _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED is not defined, which means it is now ABI-dependent if a type has to be complete.

This also means not enabling _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED stops building for us for that reason. (And building with it causes a big size regression that we're currently investigating.)

As @ldionne says in #155330 (review), _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED seems like a risky change. Maybe we shouldn't make it harder to turn it off.

Could we maybe undo this change here to make it easier to compare behavior with the new layout on and off?

[ldionne]

I wasn't aware of (and didn't expect) this breakage before I read this. It seems like the original _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED patch indeed changed whether vector requires complete types to compute empty(). I can confirm that here: https://godbolt.org/z/EMff4KPjP. This was unintended, and we should fix that at least until we decide we want to change that guarantee. I filed #210732 for that.

I think this should also resolve your concern about different requirements depending on the ABI macro.

[ldionne]

As I explained in #210732 (comment), it turns out that this is not "a guarantee" we were providing, merely something that happened to work. The Standard actually requires the type to be complete before instantiating any member function of vector<T>.

This doesn't change what I think we should do here: we should return to making it work at least for the LLVM 23 release cause this was unintended and it touches an important type -- but I suspect you should also get started fixing the code that relies on this.

[ldionne]

#210754

[nico]

Thanks for the PR!

It seems like the original _LIBCPP_ABI_VECTOR_LAYOUT_SIZE_BASED patch indeed changed whether vector requires complete types to compute empty()

Just to be overly clear (I think you understood it already), but the problem for us was that this PR made the old vector require complete types, while the new one didn't. This made it impossible for us to switch back to the old impl to debug a size increase.

Thanks again for unblocking us :)

[cjdb]

The changes in this patch undo some very delicate fine-tuning that I did in #155330. I'm disappointed that I wasn't asked to review this patch before merging.

[ldionne]

The changes in this patch undo some very delicate fine-tuning that I did in #155330.

Can you explain? When I reviewed this with Nikolas he explained that he looked at the codegen and all of these are exactly the same after the first InstCombine pass. If that's not the case, then perhaps a revert is mandated, but let's discuss the technical concerns with it.

I'm disappointed that I wasn't asked to review this patch before merging.

Unless mistaken, you are notified on libc++ PRs, so I'm not certain what your expectation was. I don't think it's realistic to put you on the critical path for every vector change going forward since you landed the size-based vector patch.