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

[Martin Liška]

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 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)
 +{

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

[Richard Biener]

On Tue, Nov 10, 2015 at 9:47 AM, Martin Liška  wrote:
> 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

[Richard Biener]

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

[Richard Biener]

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

[Martin Liška]

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
+  {
+