On Thu, Oct 24, 2024 at 8:16 AM Andrew Pinski <pins...@gmail.com> wrote:
>
> 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've been using it extensively recently, so yes - definitely.
> 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?
Yes!
> 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.
Then do not use variadic templates? Can't we use std::initializer_list or so?
> 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.
I prefer the last or the first, definitely not the second.
Alternatively have a variant where the locator needs to be explicitly
sepecified as earlier arg, like
vec.safe_emplace_push (PASS_STAT 3, 4, 5);
?
> 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
> > > >
