On Wed, Oct 23, 2024 at 12:28 AM Andrew Pinski <pins...@gmail.com> wrote:
>
> On Tue, Oct 22, 2024 at 11:49 PM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Tue, Oct 22, 2024 at 5:31 PM Andrew Pinski <quic_apin...@quicinc.com>
> > wrote:
> > >
> > > This adds quick_emplace_push and safe_emplace_push to vec.
> > > These are like std::vector's emplace_back so you don't need an extra
> > > copy of the struct around.
> > >
> > > Since we require C++11 and also support partial non-PODs for vec, these
> > > functions
> > > can be added.
> > >
> > > I will be using them to cleanup some code and even improving how some
> > > places
> > > use vec.
> > >
> > > Bootstrapped and tested on x86_64-linux-gnu.
> > >
> > > gcc/ChangeLog:
> > >
> > > * vec.h (vec<T,vl_embed>::quick_emplace_push): New function.
> > > (vec<T,vl_ptr>::quick_emplace_push): New function.
> > > (vec<T,vl_ptr>::safe_emplace_push): New function.
> > > * vec.cc (test_init): Add test for safe_emplace_push.
> > > (test_quick_emplace_push): New function.
> > > (test_safe_emplace_push): New function.
> > > (vec_cc_tests): Call test_quick_emplace_push and
> > > test_safe_emplace_push.
> > >
> > > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > > ---
> > > gcc/vec.cc | 41 +++++++++++++++++++++++++++++++++++++++++
> > > gcc/vec.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 87 insertions(+)
> > >
> > > diff --git a/gcc/vec.cc b/gcc/vec.cc
> > > index ba963b96c6a..cd7cd3c156b 100644
> > > --- a/gcc/vec.cc
> > > +++ b/gcc/vec.cc
> > > @@ -304,6 +304,10 @@ test_init ()
> > >
> > > ASSERT_EQ (2, v1.length ());
> > > ASSERT_EQ (2, v2.length ());
> > > + v2.safe_emplace_push (1);
> > > +
> > > + ASSERT_EQ (3, v1.length ());
> > > + ASSERT_EQ (3, v2.length ());
> > > v1.release ();
> > > }
> > > }
> > > @@ -327,6 +331,25 @@ test_quick_push ()
> > > ASSERT_EQ (7, v[2]);
> > > }
> > >
> > > +/* Verify that vec::quick_emplace_push works correctly. */
> > > +
> > > +static void
> > > +test_quick_emplace_push ()
> > > +{
> > > + auto_vec <std::pair<int,int>> v;
> > > + ASSERT_EQ (0, v.length ());
> > > + v.reserve (3);
> > > + ASSERT_EQ (0, v.length ());
> > > + ASSERT_TRUE (v.space (3));
> > > + v.quick_emplace_push (5, 5);
> > > + v.quick_emplace_push (6, 6);
> > > + v.quick_emplace_push (7, 7);
> > > + ASSERT_EQ (3, v.length ());
> > > + ASSERT_EQ (std::make_pair(5,5), v[0]);
> > > + ASSERT_EQ (std::make_pair(6,6), v[1]);
> > > + ASSERT_EQ (std::make_pair(7,7), v[2]);
> > > +}
> > > +
> > > /* Verify that vec::safe_push works correctly. */
> > >
> > > static void
> > > @@ -343,6 +366,22 @@ test_safe_push ()
> > > ASSERT_EQ (7, v[2]);
> > > }
> > >
> > > +/* Verify that vec::safe_emplace_push works correctly. */
> > > +
> > > +static void
> > > +test_safe_emplace_push ()
> > > +{
> > > + auto_vec <std::pair<int,int>> v;
> > > + ASSERT_EQ (0, v.length ());
> > > + v.safe_emplace_push (5, 5);
> > > + v.safe_emplace_push (6, 6);
> > > + v.safe_emplace_push (7, 7);
> > > + ASSERT_EQ (3, v.length ());
> > > + ASSERT_EQ (std::make_pair(5,5), v[0]);
> > > + ASSERT_EQ (std::make_pair(6,6), v[1]);
> > > + ASSERT_EQ (std::make_pair(7,7), v[2]);
> > > +}
> > > +
> > > /* Verify that vec::truncate works correctly. */
> > >
> > > static void
> > > @@ -591,7 +630,9 @@ vec_cc_tests ()
> > > {
> > > test_init ();
> > > test_quick_push ();
> > > + test_quick_emplace_push ();
> > > test_safe_push ();
> > > + test_safe_emplace_push ();
> > > test_truncate ();
> > > test_safe_grow_cleared ();
> > > test_pop ();
> > > diff --git a/gcc/vec.h b/gcc/vec.h
> > > index b13c4716428..8277d156f05 100644
> > > --- a/gcc/vec.h
> > > +++ b/gcc/vec.h
> > > @@ -619,6 +619,8 @@ public:
> > > void splice (const vec &);
> > > void splice (const vec *src);
> > > T *quick_push (const T &);
> > > + template <typename ...Args>
> > > + T *quick_emplace_push (Args&&... args);
> > > using pop_ret_type
> > > = typename std::conditional <std::is_trivially_destructible
> > > <T>::value,
> > > T &, void>::type;
> > > @@ -1044,6 +1046,21 @@ vec<T, A, vl_embed>::quick_push (const T &obj)
> > > return slot;
> > > }
> > >
> > > +/* Push T(ARGS) (a new element) onto the end of the vector. There must
> > > be
> > > + sufficient space in the vector. Return a pointer to the slot
> > > + where T(ARGS) was inserted. */
> > > +
> > > +template<typename T, typename A>
> > > +template<typename ...Args>
> > > +inline T *
> > > +vec<T, A, vl_embed>::quick_emplace_push (Args&&... args)
> > > +{
> > > + gcc_checking_assert (space (1));
> > > + T *slot = &address ()[m_vecpfx.m_num++];
> > > + ::new (static_cast<void*>(slot)) T (std::forward<Args>(args)...);
> > > + return slot;
> > > +}
> > > +
> > >
> > > /* Pop and return a reference to the last element off the end of the
> > > vector. If T has non-trivial destructor, this method just pops
> > > @@ -1612,7 +1629,11 @@ public:
> > > void splice (const vec &);
> > > void safe_splice (const vec & CXX_MEM_STAT_INFO);
> > > T *quick_push (const T &);
> > > + template <typename ...Args>
> > > + T *quick_emplace_push (Args&&... args);
> > > T *safe_push (const T &CXX_MEM_STAT_INFO);
> > > + template <typename ...Args>
> > > + T *safe_emplace_push (Args&&... args CXX_MEM_STAT_INFO);
> > > using pop_ret_type
> > > = typename std::conditional <std::is_trivially_destructible
> > > <T>::value,
> > > T &, void>::type;
> > > @@ -2070,6 +2091,18 @@ vec<T, va_heap, vl_ptr>::quick_push (const T &obj)
> > > return m_vec->quick_push (obj);
> > > }
> > >
> > > +/* Push OBJ (a new element) onto the end of the vector. There must be
> > > + sufficient space in the vector. Return a pointer to the slot
> > > + where OBJ was inserted. */
> > > +
> > > +template<typename T>
> > > +template <typename ...Args>
> > > +inline T *
> > > +vec<T, va_heap, vl_ptr>::quick_emplace_push (Args&& ... args)
> > > +{
> > > + return m_vec->quick_emplace_push (std::forward<Args>(args)...);
> > > +}
> > > +
> > >
> > > /* Push a new element OBJ onto the end of this vector. Reallocates
> > > the embedded vector, if needed. Return a pointer to the slot where
> > > @@ -2083,6 +2116,19 @@ vec<T, va_heap, vl_ptr>::safe_push (const T &obj
> > > MEM_STAT_DECL)
> > > return quick_push (obj);
> > > }
> > >
> > > +/* Push a new element T(ARGS) onto the end of this vector. Reallocates
> > > + the embedded vector, if needed. Return a pointer to the slot where
> > > + OBJ was inserted. */
> > > +
> > > +template<typename T>
> > > +template <typename ...Args>
> > > +inline T *
> > > +vec<T, va_heap, vl_ptr>::safe_emplace_push (Args&&... args MEM_STAT_DECL)
> >
> > Did you check this works with --enable-gather-detailed-mem-stats?
>
> Whoops, let me test that. It might be the case it needs to be in the
> front because of the way variadic templates work.
So the answer is No, it does not work. In recent years has these
stats been useful? I know it is useful to know how much a pass
allocates to make sure it does not go off to the weeds but recording
where the allocation in the pass have recently helped?
Rather I am asking should we just forebode recording them for
safe_emplace_push since there is no way for variadic templates and
default arguments to work together.
Though we could just declare there are at max 3 arguments to a
constructor and not do variadic templates. We can add more if needed
but I think having one for 0, 1, 2, and 3 arguments will be enough for
most constructors (pair is 2; I have an usage that has 0 arguments
that I am working on).
So the 3 options forward I see are:
* drop the patch and just keep what is happening today
* Don't pass the stats
* Have a fixed number of arguments for safe_emplace_push (0, 1, 2, and
3) and have a variable number still for quick_emplace_push (since
there is no allocation done).
I prefer the last one but the middle one is ok too.
Thanks,
Andrew Pinski
>
> Thanks,
> Andrew
>
> >
> > > +{
> > > + reserve (1, false PASS_MEM_STAT);
> > > + return quick_emplace_push (std::forward<Args>(args)...);
> > > +}
> > > +
> > >
> > > /* Pop and return a reference to the last element off the end of the
> > > vector. If T has non-trivial destructor, this method just pops
> > > --
> > > 2.43.0
> > >
