This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
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)