This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: std::vector move assign patch


I plan to propose this patch when back to stage 1. As it defaults default and move constructor I wonder if it can make any change to the generated code.

François


On 25/04/2017 23:02, Marc Glisse wrote:
On Tue, 25 Apr 2017, Jonathan Wakely wrote:

That's only 5 more memory operations. If I tweak vector swapping to avoid calling swap on each member (which drops type-based aliasing information, that was the topic of PR64601)

I didn't really understand the discussion in the PR. I find that's
true of most TBAA discussions.

std::swap(T& x, T& y) is hard to optimise because we don't know that
the dynamic type of the thing at &x is the same type as T?

std::swap<int*> only knows that it is dealing with pointers to integers, so _M_start from one vector might be in the same location as _M_finish from some other vector...

When I access __x._M_start directly, I am accessing some part of a _Vector_impl, and 2 _Vector_impl can only be the same or disjoint, they cannot partially overlap.

I see. I'm surprised that after the std::swap calls get inlined, so
that the references can be "seen through", the pessimisation is still
present.

(this is my understanding of how compilers handle TBAA)

The only things that carry information are the declaration of an object, and accesses (read/write). Pointers and references are meaningless until the object is accessed, they are just a fancy way to do pointer arithmetic, and the way the object is accessed carries the whole information. If I have a reference p to std::pair<int,int>, p.first=42 or x=p.first access the first element of a pair, while *&p.first=42 or x=*&p.first are only accesses to an int. p.first and q.second cannot alias, while *&p.first and *&q.second can.

In other words
illegal: ((std::pair<int,int>*)&p.second)->first=2
allowed: *(int*)&((std::pair<int,int>*)&p.second)->first=2
because there is no actual access through the "wrong" pointers (I added a redundant cast to int* to try and clarify the difference).

(actually, gcc wrongly optimizes *&x to x so you need to use an intermediate variable to notice this: auto y=&x; ... *y ... while clang is stricter)

I think this is horrible but that's not a fight I feel capable of leading.

If what I said is too hard to understand, it is fine to ignore. And if it is wrong, I'll be happy to learn that :-)

By the way, do we have a policy on writing if(p)delete p; vs directly delete p; which does nothing for p==0? There are cases where the extra shortcut is a nice optimization, others where it is redundant work. At -Os, the middle-end could optimize if(p)free(p) to free(p) in the future.

No official policy. I always consider the if (p) to be redundant
noise, but that's based on readability and correctness, not
performance.

I was going to say we have that in std::vector's destructor, but allocators do not have the same property as free/delete, deallocate can only be called with something allocate returned, not 0. So we don't have a choice.


diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index fb88212..9b10eba 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -78,29 +78,29 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       typedef typename __gnu_cxx::__alloc_traits<_Tp_alloc_type>::pointer
        	pointer;
 
-      struct _Vector_impl
-      : public _Tp_alloc_type
+      struct _Vector_impl_data
       {
 	pointer _M_start;
 	pointer _M_finish;
 	pointer _M_end_of_storage;
 
-	_Vector_impl()
-	: _Tp_alloc_type(), _M_start(), _M_finish(), _M_end_of_storage()
-	{ }
-
-	_Vector_impl(_Tp_alloc_type const& __a) _GLIBCXX_NOEXCEPT
-	: _Tp_alloc_type(__a), _M_start(), _M_finish(), _M_end_of_storage()
+	_Vector_impl_data() _GLIBCXX_NOEXCEPT
+	: _M_start(), _M_finish(), _M_end_of_storage()
 	{ }
 
 #if __cplusplus >= 201103L
-	_Vector_impl(_Tp_alloc_type&& __a) noexcept
-	: _Tp_alloc_type(std::move(__a)),
-	  _M_start(), _M_finish(), _M_end_of_storage()
-	{ }
+	_Vector_impl_data(_Vector_impl_data&& __x) noexcept
+	: _M_start(__x._M_start), _M_finish(__x._M_finish),
+	  _M_end_of_storage(__x._M_end_of_storage)
+	{ __x._M_reset(); }
+
+	void
+	_M_reset() noexcept
+	{ _M_start = _M_finish = _M_end_of_storage = pointer(); }
 #endif
 
-	void _M_swap_data(_Vector_impl& __x) _GLIBCXX_NOEXCEPT
+	void
+	_M_swap_data(_Vector_impl_data& __x) _GLIBCXX_NOEXCEPT
 	{
 	  std::swap(_M_start, __x._M_start);
 	  std::swap(_M_finish, __x._M_finish);
@@ -108,6 +108,28 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	}
       };
 
+      struct _Vector_impl
+	: public _Tp_alloc_type, public _Vector_impl_data
+      {
+#if __cplusplus >= 201103L
+	_Vector_impl() = default;
+#else
+	_Vector_impl()
+	: _Tp_alloc_type()
+	{ }
+#endif
+
+	_Vector_impl(_Tp_alloc_type const& __a) _GLIBCXX_NOEXCEPT
+	: _Tp_alloc_type(__a)
+	{ }
+
+#if __cplusplus >= 201103L
+	_Vector_impl(_Tp_alloc_type&& __a) noexcept
+	: _Tp_alloc_type(std::move(__a))
+	{ }
+#endif
+      };
+
     public:
       typedef _Alloc allocator_type;
 
@@ -123,8 +145,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       get_allocator() const _GLIBCXX_NOEXCEPT
       { return allocator_type(_M_get_Tp_allocator()); }
 
+#if __cplusplus >= 201103L
+	_Vector_base() = default;
+#else
       _Vector_base()
       : _M_impl() { }
+#endif
 
       _Vector_base(const allocator_type& __a) _GLIBCXX_NOEXCEPT
       : _M_impl(__a) { }
@@ -141,9 +167,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _Vector_base(_Tp_alloc_type&& __a) noexcept
       : _M_impl(std::move(__a)) { }
 
-      _Vector_base(_Vector_base&& __x) noexcept
-      : _M_impl(std::move(__x._M_get_Tp_allocator()))
-      { this->_M_impl._M_swap_data(__x._M_impl); }
+      _Vector_base(_Vector_base&&) = default;
 
       _Vector_base(_Vector_base&& __x, const allocator_type& __a)
       : _M_impl(__a)
@@ -258,9 +282,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
       vector()
 #if __cplusplus >= 201103L
-      noexcept(is_nothrow_default_constructible<_Alloc>::value)
-#endif
+      = default;
+#else
       : _Base() { }
+#endif
 
       /**
        *  @brief  Creates a %vector with no elements.
@@ -336,13 +361,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 #if __cplusplus >= 201103L
       /**
        *  @brief  %Vector move constructor.
-       *  @param  __x  A %vector of identical element and allocator types.
        *
-       *  The newly-created %vector contains the exact contents of @a __x.
-       *  The contents of @a __x are a valid, but unspecified %vector.
+       *  The newly-created %vector contains the exact contents of the
+       *  moved instance.
+       *  The contents of the moved instance are a valid, but unspecified
+       *  %vector.
        */
-      vector(vector&& __x) noexcept
-      : _Base(std::move(__x)) { }
+      vector(vector&&) = default;
 
       /// Copy constructor with alternative allocator
       vector(const vector& __x, const allocator_type& __a)

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]