On Thu, Oct 24, 2024 at 6:23 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> 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?
std::initializer_list only works if all of the arguments types are the same.
>
> > 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);
We could do that with an additional empty struct for the non
`--enable-gather-detailed-mem-stats` case to make sure that option
does not break.
The empty struct for many targets will be ignored for argument passing
but these will most likely be inlined anyways so it should not be a
big deal.
I will code this up and see how it works out.
>
> ?
>
> > 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
> > > > >
