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

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

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;


> 
> 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);

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

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
+  {
+    return m_allocator.allocate ();
+  }
 
   inline void
   remove (T *object)
@@ -528,7 +539,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.  */