On Tue, Nov 10, 2015 at 9:47 AM, Martin Liška <mli...@suse.cz> 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<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?
Ok.
Thanks,
Richard.
> 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. */
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
