Re: [hsa 9/12] Small alloc-pool fix
On 11/06/2015 10:57 AM, Richard Biener wrote: > 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::allocate() [with T = et_occ]’: >> ../../gcc/alloc-pool.h:531:22: required from ‘void* operator new(size_t, >> object_allocator&) [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 >> inline void * >> operator new (size_t, object_allocator ) >> { >> 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. Hi. I'm sending suggested patch that survives regression tests and bootstrap on x86_64-linux-gnu. Can I install the patch to trunk? Thanks, Martin > > Thanks, > Richard. > >> Thanks, >> Martin >> >>> >>> Thanks. >>> Richard. >>> Thanks, Martin 2015-11-05 Martin LiskaMartin Jambor * 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 inline void * operator new (size_t, object_allocator ) { - return a.allocate (); + return a.vallocate (); +} + +/* Helper for classes that do not provide default ctor. */ +template +inline void * +operator new (size_t, object_allocator *a) +{
Re: [hsa 9/12] Small alloc-pool fix
On Tue, Nov 10, 2015 at 9:47 AM, Martin Liškawrote: > On 11/06/2015 10:57 AM, Richard Biener wrote: >> 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::allocate() [with T = et_occ]’: >>> ../../gcc/alloc-pool.h:531:22: required from ‘void* operator new(size_t, >>> object_allocator&) [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 >>> inline void * >>> operator new (size_t, object_allocator ) >>> { >>> 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. > > Hi. > > I'm sending suggested patch that survives regression tests and bootstrap > on x86_64-linux-gnu. > > Can I install the patch to trunk? Ok. Thanks, Richard. > Thanks, > Martin > >> >> Thanks, >> Richard. >> >>> Thanks, >>> Martin >>> Thanks. Richard. > Thanks, > > Martin > > > 2015-11-05 Martin Liska >Martin Jambor > >* 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 > inline void * > operator new (size_t,
Re: [hsa 9/12] Small alloc-pool fix
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::allocate() [with T = et_occ]’: > ../../gcc/alloc-pool.h:531:22: required from ‘void* operator new(size_t, > object_allocator&) [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 > inline void * > operator new (size_t, object_allocator ) > { > 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> >>Martin Jambor > >> > >>* 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 > >> inline void * > >> operator new (size_t, object_allocator ) > >> { > >> - return a.allocate (); > >> + return a.vallocate (); > >> +} > >> + > >> +/* Helper for classes that do not provide default ctor. */ > >> +template > >> +inline void * > >> +operator new (size_t, object_allocator *a) > >> +{ > >> + return a->vallocate (); > >> } > >> > >> /* Hashtable mapping alloc_pool names to descriptors. */ > >> > >> > > > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
Re: [hsa 9/12] Small alloc-pool fix
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. Why do you need to add the pointer variant then? 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. 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; thus default-initialize which does no initialization for PODs (without array members...) which is what the old pool allocator did. 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. Thanks. Richard. > Thanks, > > Martin > > > 2015-11-05 Martin Liska> Martin Jambor > > * 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 > inline void * > operator new (size_t, object_allocator ) > { > - return a.allocate (); > + return a.vallocate (); > +} > + > +/* Helper for classes that do not provide default ctor. */ > +template > +inline void * > +operator new (size_t, object_allocator *a) > +{ > + return a->vallocate (); > } > > /* Hashtable mapping alloc_pool names to descriptors. */ > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Re: [hsa 9/12] Small alloc-pool fix
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::allocate() [with T = et_occ]’: ../../gcc/alloc-pool.h:531:22: required from ‘void* operator new(size_t, object_allocator&) [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; > > 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 inline void * operator new (size_t, object_allocator ) { return a.allocate (); } For instance: et_occ *nw = new (et_occurrences) et_occ (2); 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. Thanks, Martin > > Thanks. > Richard. > >> Thanks, >> >> Martin >> >> >> 2015-11-05 Martin Liska>> Martin Jambor >> >> * 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 >> inline void * >> operator new (size_t, object_allocator ) >> { >> - return a.allocate (); >> + return a.vallocate (); >> +} >> + >> +/* Helper for classes that do not provide default ctor. */ >> +template >> +inline void * >> +operator new (size_t, object_allocator *a) >> +{ >> + return a->vallocate (); >> } >> >> /* Hashtable mapping alloc_pool names to descriptors. */ >> >> > diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h index 0dc05cd..8b8c023 100644 --- a/gcc/alloc-pool.h +++ b/gcc/alloc-pool.h @@ -477,11 +477,22 @@ public: m_allocator.release_if_empty (); } + /* Allocate memory for instance of type T and call a default constructor. */ + inline T * allocate () ATTRIBUTE_MALLOC { return ::new (m_allocator.allocate ()) T (); } + /* Allocate memory for instance of type T and return void * that + could be used in situations where a default constructor is not provided + by the class T. */ + + inline void * + allocate_raw () ATTRIBUTE_MALLOC + { +