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 <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. */
>>>>
>>>>
>>>
>>
>>
>
>From acccc1c0f4bfe38b14c7dcc2c278c63b6484e91b Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Mon, 9 Nov 2015 16:52:21 +0100
Subject: [PATCH] Enhance pool allocator
gcc/ChangeLog:
2015-11-09 Martin Liska <mli...@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
