Re: [hsa 9/12] Small alloc-pool fix

[Richard Biener <rguenther at suse dot de>]

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  <mliska@suse.cz>
> >> 	    Martin Jambor  <mjambor@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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)