public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jonathan Wakely <jwakely@redhat.com>
To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: [committed] libstdc++: Fix code size regressions in std::vector [PR110060]
Date: Thu,  1 Jun 2023 16:09:58 +0100	[thread overview]
Message-ID: <20230601150958.268109-1-jwakely@redhat.com> (raw)

Tested powerpc64le-linux. Pusshed to trunk.

-- >8 --

My r14-1452-gfb409a15d9babc change to add optimization hints to
std::vector causes regressions because it makes std::vector::size() and
std::vector::capacity() too big to inline. That's the opposite of what
I wanted, so revert the changes to those functions.

To achieve the original aim of optimizing vec.assign(vec.size(), x) we
can add a local optimization hint to _M_fill_assign, so that it doesn't
affect all other uses of size() and capacity().

Additionally, add the same hint to the _M_assign_aux overload for
forward iterators and add that to the testcase.

It would be nice to similarly optimize:
  if (vec1.size() == vec2.size()) vec1 = vec2;
but adding hints to operator=(const vector&) doesn't help. Presumably
the relationships between the two sizes and two capacities are too
complex to track effectively.

libstdc++-v3/ChangeLog:

	PR libstdc++/110060
	* include/bits/stl_vector.h (_Vector_base::_M_invariant):
	Remove.
	(vector::size, vector::capacity): Remove calls to _M_invariant.
	* include/bits/vector.tcc (vector::_M_fill_assign): Add
	optimization hint to reallocating path.
	(vector::_M_assign_aux(FwdIter, FwdIter, forward_iterator_tag)):
	Likewise.
	* testsuite/23_containers/vector/capacity/invariant.cc: Moved
	to...
	* testsuite/23_containers/vector/modifiers/assign/no_realloc.cc:
	...here. Check assign(FwdIter, FwdIter) too.
	* testsuite/23_containers/vector/types/1.cc: Revert addition
	of -Wno-stringop-overread option.
---
 libstdc++-v3/include/bits/stl_vector.h        | 23 +------------------
 libstdc++-v3/include/bits/vector.tcc          | 17 ++++++++++----
 .../assign/no_realloc.cc}                     |  6 +++++
 .../testsuite/23_containers/vector/types/1.cc |  2 +-
 4 files changed, 20 insertions(+), 28 deletions(-)
 rename libstdc++-v3/testsuite/23_containers/vector/{capacity/invariant.cc => modifiers/assign/no_realloc.cc} (70%)

diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index e593be443bc..70ced3d101f 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -389,23 +389,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
     protected:
 
-      __attribute__((__always_inline__))
-      _GLIBCXX20_CONSTEXPR void
-      _M_invariant() const
-      {
-#if __OPTIMIZE__
-	if (this->_M_impl._M_finish < this->_M_impl._M_start)
-	  __builtin_unreachable();
-	if (this->_M_impl._M_finish > this->_M_impl._M_end_of_storage)
-	  __builtin_unreachable();
-
-	size_t __sz = this->_M_impl._M_finish - this->_M_impl._M_start;
-	size_t __cap = this->_M_impl._M_end_of_storage - this->_M_impl._M_start;
-	if (__sz > __cap)
-	  __builtin_unreachable();
-#endif
-      }
-
       _GLIBCXX20_CONSTEXPR
       void
       _M_create_storage(size_t __n)
@@ -1005,10 +988,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
       size_type
       size() const _GLIBCXX_NOEXCEPT
-      {
-	_Base::_M_invariant();
-	return size_type(this->_M_impl._M_finish - this->_M_impl._M_start);
-      }
+      { return size_type(this->_M_impl._M_finish - this->_M_impl._M_start); }
 
       /**  Returns the size() of the largest possible %vector.  */
       _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
@@ -1095,7 +1075,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       size_type
       capacity() const _GLIBCXX_NOEXCEPT
       {
-	_Base::_M_invariant();
 	return size_type(this->_M_impl._M_end_of_storage
 			   - this->_M_impl._M_start);
       }
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index d6fdea2dd01..acd11e2dc68 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -270,15 +270,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
     vector<_Tp, _Alloc>::
     _M_fill_assign(size_t __n, const value_type& __val)
     {
+      const size_type __sz = size();
       if (__n > capacity())
 	{
+	  if (__n <= __sz)
+	    __builtin_unreachable();
 	  vector __tmp(__n, __val, _M_get_Tp_allocator());
 	  __tmp._M_impl._M_swap_data(this->_M_impl);
 	}
-      else if (__n > size())
+      else if (__n > __sz)
 	{
 	  std::fill(begin(), end(), __val);
-	  const size_type __add = __n - size();
+	  const size_type __add = __n - __sz;
 	  _GLIBCXX_ASAN_ANNOTATE_GROW(__add);
 	  this->_M_impl._M_finish =
 	    std::__uninitialized_fill_n_a(this->_M_impl._M_finish,
@@ -316,10 +319,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _M_assign_aux(_ForwardIterator __first, _ForwardIterator __last,
 		    std::forward_iterator_tag)
       {
+	const size_type __sz = size();
 	const size_type __len = std::distance(__first, __last);
 
 	if (__len > capacity())
 	  {
+	    if (__len <= __sz)
+	      __builtin_unreachable();
+
 	    _S_check_init_len(__len, _M_get_Tp_allocator());
 	    pointer __tmp(_M_allocate_and_copy(__len, __first, __last));
 	    std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish,
@@ -332,14 +339,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	    this->_M_impl._M_finish = this->_M_impl._M_start + __len;
 	    this->_M_impl._M_end_of_storage = this->_M_impl._M_finish;
 	  }
-	else if (size() >= __len)
+	else if (__sz >= __len)
 	  _M_erase_at_end(std::copy(__first, __last, this->_M_impl._M_start));
 	else
 	  {
 	    _ForwardIterator __mid = __first;
-	    std::advance(__mid, size());
+	    std::advance(__mid, __sz);
 	    std::copy(__first, __mid, this->_M_impl._M_start);
-	    const size_type __attribute__((__unused__)) __n = __len - size();
+	    const size_type __attribute__((__unused__)) __n = __len - __sz;
 	    _GLIBCXX_ASAN_ANNOTATE_GROW(__n);
 	    this->_M_impl._M_finish =
 	      std::__uninitialized_copy_a(__mid, __last,
diff --git a/libstdc++-v3/testsuite/23_containers/vector/capacity/invariant.cc b/libstdc++-v3/testsuite/23_containers/vector/modifiers/assign/no_realloc.cc
similarity index 70%
rename from libstdc++-v3/testsuite/23_containers/vector/capacity/invariant.cc
rename to libstdc++-v3/testsuite/23_containers/vector/modifiers/assign/no_realloc.cc
index d68db694add..659f18dba56 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/capacity/invariant.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/modifiers/assign/no_realloc.cc
@@ -1,5 +1,6 @@
 // { dg-do compile }
 // { dg-options "-O3 -g0" }
+// { dg-require-normal-mode "" }
 // { dg-final { scan-assembler-not "_Znw" } }
 // GCC should be able to optimize away the paths involving reallocation.
 
@@ -14,3 +15,8 @@ void fill_val(std::vector<int>& vec, int i)
 {
   vec.assign(vec.size(), i);
 }
+
+void fill_range(std::vector<int>& vec, const int* first)
+{
+  vec.assign(first, first + vec.size());
+}
diff --git a/libstdc++-v3/testsuite/23_containers/vector/types/1.cc b/libstdc++-v3/testsuite/23_containers/vector/types/1.cc
index 9be07d9fd5c..079e5af9556 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/types/1.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/types/1.cc
@@ -18,7 +18,7 @@
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-options "-Wno-unused-result -Wno-stringop-overread" }
+// { dg-options "-Wno-unused-result" }
 
 #include <vector>
 #include <testsuite_greedy_ops.h>
-- 
2.40.1


             reply	other threads:[~2023-06-01 15:10 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-01 15:09 Jonathan Wakely [this message]
2023-06-08  8:57 ` Maxim Kuvyrkov
2023-06-08  9:05   ` Jonathan Wakely
2023-06-08  9:13     ` Jakub Jelinek
2023-06-09  9:00       ` Richard Biener
2023-06-08 11:43   ` Jonathan Wakely

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230601150958.268109-1-jwakely@redhat.com \
    --to=jwakely@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=libstdc++@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).