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 <mli...@suse.cz>
>> Martin Jambor <mjam...@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. */
