On Fri, 6 Nov 2015, Martin Liška wrote:
> On 11/06/2015 10:00 AM, Richard Biener wrote:
> > On Thu, 5 Nov 2015, Martin Jambor wrote:
> >
> >> Hi,
> >>
> >> we use C++ new operators based on alloc-pools a lot in the subsequent
> >> patches and realized that on the current trunk, such new operators
> >> would needlessly call the placement ::new operator within the allocate
> >> method of pool-alloc. Fixed below by providing a new allocation
> >> method which does not call placement new, which is only safe to use
> >> from within a new operator.
> >>
> >> The patch also fixes the slightly weird two parameter operator new
> >> (which we do not use in HSA backend) so that it does not do the same.
> >
>
> Hi.
>
> > Why do you need to add the pointer variant then?
>
> You are right, we originally used the variant in the branch, but it was
> eventually
> left.
>
> >
> > Also isn't the issue with allocate() that it does
> >
> > return ::new (m_allocator.allocate ()) T ();
> >
> > which 1) value-initializes and 2) doesn't even work with types like
> >
> > struct T { T(int); };
> >
> > thus types without a default constructor.
>
> You are right, it produces compilation error.
>
> >
> > I think the allocator was poorly C++-ified without updating the
> > specification for the cases it is supposed to handle. And now
> > we have C++ uses that are not working because the allocator is
> > broken.
> >
> > An incrementally better version (w/o fixing the issue with
> > types w/o default constructor) is
> >
> > return ::new (m_allocator.allocate ()) T;
>
> I've tried that, and it also calls default ctor:
>
> ../../gcc/alloc-pool.h: In instantiation of ‘T*
> object_allocator<T>::allocate() [with T = et_occ]’:
> ../../gcc/alloc-pool.h:531:22: required from ‘void* operator new(size_t,
> object_allocator<T>&) [with T = et_occ; size_t = long unsigned int]’
> ../../gcc/et-forest.c:449:46: required from here
> ../../gcc/et-forest.c:58:3: error: ‘et_occ::et_occ()’ is private
> et_occ ();
> ^
> In file included from ../../gcc/et-forest.c:28:0:
> ../../gcc/alloc-pool.h:483:44: error: within this context
> return ::new (m_allocator.allocate ()) T;
Yes, but it does slightly cheaper initialization of PODs
>
> >
> > thus default-initialize which does no initialization for PODs (without
> > array members...) which is what the old pool allocator did.
>
> I'm not so familiar with differences related to PODs.
>
> >
> > To fix the new operator (how do you even call that? does it allow
> > specifying constructor args and thus work without a default constructor?)
> > it should indeed use an allocation method not performing the placement
> > new. But I'd call it allocate_raw rather than vallocate.
>
> For situations where do not have a default ctor, one should you the
> helper method defined at the end of alloc-pool.h:
>
> template <typename T>
> inline void *
> operator new (size_t, object_allocator<T> &a)
> {
> return a.allocate ();
> }
>
> For instance:
> et_occ *nw = new (et_occurrences) et_occ (2);
Oh, so it uses placement new syntax... works for me.
> or as used in the HSA branch:
>
> /* New operator to allocate convert instruction from pool alloc. */
>
> void *
> hsa_insn_cvt::operator new (size_t)
> {
> return hsa_allocp_inst_cvt->allocate_raw ();
> }
>
> and
>
> cvtinsn = new hsa_insn_cvt (reg, *ptmp2);
>
>
> I attached patch where I rename the method as suggested.
Ok.
Thanks,
Richard.
> Thanks,
> Martin
>
> >
> > Thanks.
> > Richard.
> >
> >> Thanks,
> >>
> >> Martin
> >>
> >>
> >> 2015-11-05 Martin Liska <mli...@suse.cz>
> >> Martin Jambor <mjam...@suse.cz>
> >>
> >> * alloc-pool.h (object_allocator::vallocate): New method.
> >> (operator new): Call vallocate instead of allocate.
> >> (operator new): New operator.
> >>
> >>
> >> diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
> >> index 0dc05cd..46b6550 100644
> >> --- a/gcc/alloc-pool.h
> >> +++ b/gcc/alloc-pool.h
> >> @@ -483,6 +483,12 @@ public:
> >> return ::new (m_allocator.allocate ()) T ();
> >> }
> >>
> >> + inline void *
> >> + vallocate () ATTRIBUTE_MALLOC
> >> + {
> >> + return m_allocator.allocate ();
> >> + }
> >> +
> >> inline void
> >> remove (T *object)
> >> {
> >> @@ -523,12 +529,19 @@ struct alloc_pool_descriptor
> >> };
> >>
> >> /* Helper for classes that do not provide default ctor. */
> >> -
> >> template <typename T>
> >> inline void *
> >> operator new (size_t, object_allocator<T> &a)
> >> {
> >> - return a.allocate ();
> >> + return a.vallocate ();
> >> +}
> >> +
> >> +/* Helper for classes that do not provide default ctor. */
> >> +template <typename T>
> >> +inline void *
> >> +operator new (size_t, object_allocator<T> *a)
> >> +{
> >> + return a->vallocate ();
> >> }
> >>
> >> /* Hashtable mapping alloc_pool names to descriptors. */
> >>
> >>
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
