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

[Martin LiÅka <mliska at suse dot cz>]

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<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.

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  <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.  */
>>>>
>>>>
>>>
>>
>>
> 

>From acccc1c0f4bfe38b14c7dcc2c278c63b6484e91b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 9 Nov 2015 16:52:21 +0100
Subject: [PATCH] Enhance pool allocator

gcc/ChangeLog:

2015-11-09  Martin Liska  <mliska@suse.cz>

	* alloc-pool.h (allocate_raw): New function.
	(operator new (size_t, object_allocator<T> &a)): Use the
	function instead of object_allocator::allocate).
---
 gcc/alloc-pool.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index bf9b0eb..38aff28 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -477,12 +477,25 @@ 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
+  {
+    return m_allocator.allocate ();
+  }
+
   inline void
   remove (T *object)
   {
@@ -528,7 +541,7 @@ template <typename T>
 inline void *
 operator new (size_t, object_allocator<T> &a)
 {
-  return a.allocate ();
+  return a.allocate_raw ();
 }
 
 /* Hashtable mapping alloc_pool names to descriptors.  */
-- 
2.6.2