On Tue, 16 Oct 2012, Diego Novillo wrote:
> I have overhauled the interface to VEC over the last few
> weeks. I implemented most of the plan I outlined in
> http://gcc.gnu.org/ml/gcc/2012-08/msg00268.html.
>
> I have implemented the embedded and space-efficient vectors. The
> diff for vec.[ch] is sufficiently confusing that I'm just
> attaching both files for review.
>
> In this message, I want to outline the major changes I've done.
> The patch still does not work (gengtype is giving me a LOT of
> problems because vectors are not pointers anymore).
>
> There are two new types:
>
> 1- Embedded vectors: vec_e<element_type>
>
> These are fixed-size vectors useful to be embedded in
> structures. For instance, tree_binfo::base_binfos. They use
> the trailing array idiom. Once allocated, they cannot be
> re-sized.
>
>
> 2- Space efficient vectors: vec_s<element_type, allocation>
>
> These are the traditional VECs we have always used. They are
> implemented as a pointer to a vec_e<element_type>. The
> difference with traditional VECs is that they no longer need
> to be pointer themselves, so their address does not change if
> the internal vector needs to be re-allocated. They still use
> just a single word of storage before allocation, so they can
> be embedded in data structures without altering their size.
Why change the name? I'd have used vec<> ...
Can't we implement vec_e as vec<element_type, embedded> by
means of a partial specialization?
Sorry for the bike-shedding ;)
> I have not yet implemented the "fast" vectors from my proposal.
> Those would be useful for free variables or structures that can
> tolerate an extra word of storage.
>
> Needless to say the patch is gigantic: 2.4 Mb, 334 files
> changed, 10718 insertions(+), 11536 deletions(-). But it is
> largely mechanic.
>
> We no longer access the vectors via VEC_* macros. The pattern is
> "VEC_operation (T, A, V, args)" becomes "V.operation (args)".
> That is mostly why you see ~800 fewer lines in the patch.
>
> The only thing I could not do is create proper ctors and dtors
> for the vec_s/vec_e classes. Since these vectors are stored in
> unions, we have to keep them as PODs (C++03 does not allow
> non-PODs in unions).
>
> This means that creation and destruction must be explicit. There
> is a new method vec_s<type, allocation>::create() and another
> vec_s<type, allocation>::destroy() to allocate the internal
> vector.
::alloc() and ::free()? I see you have those, too, but from
/* Vector allocation. */
static vec_s<T, A> *alloc (unsigned CXX_MEM_STAT_INFO);
static void free (vec_s<T, A> *&);
/* Internal vector creation and initialization. */
static vec_s<T, A> create (unsigned CXX_MEM_STAT_INFO);
static vec_s<T, A> create (unsigned, vec_e<T> *);
void destroy (void);
I can't see how they differ and what users should use? I suppose
::create and ::destroy are just implementation details? Thus
you'd make them private? Why is create not a member function?
It should be IMHO
template<typename T, enum vec_allocation A>
inline vec_s<T, A>&
vec_s<T, A>::create (unsigned nelems, vec_e<T> *space)
{
gcc_assert (A == va_stack);
this->stack_reserve (space, nelems);
return *this;
}
Otherwise please use gcc_checking_assert in inline functions,
otherwise they won't be inlined due to size constraints in most
of the cases.
Likewise ::copy should mimic a copy constructor. Not sure if
that will work out ... isn't it enough that vec_e is embeddable
into a union?
Otherwise thanks for working on this and sorry for the C++
bikeshedding - there are just so many more ways to do something
in C++ compared to C ;)
Richard.
> My next steps are:
>
> - Fix gengtype issues. Since the GTY(()) vectors are not
> pointers, gengtype is ignoring them, not adding roots and
> marking functions. I think I've almost fixed it, but it will
> take me another day or two.
>
> - Test on all affected architectures and compilation modes.
>
> - Make sure memory consumption and performance are still on par
> with the current implementation.
>
> Things I want to change (that I may do in subsequent patches):
>
> - The allocation strategy ought to be an allocator type not an
> enum.
>
> - vec_s<T, A>::iterate should disappear. Instead, we want to use
> standard iterator idioms.
>
>
> Please take a look at vec.[ch]. I have also pushed my changes to
> the git branch git://gcc.gnu.org/git/gcc.git/dnovillo/vec-rewrite
> (it's based on today's trunk).
>
> Comments?
>
>
> Thanks. Diego.
>
--
Richard Biener <[email protected]>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend
