[PATCH] Introduce needsCleanup() for APFloat and APInt

[Manuel Klimek]

Richard, anything to add? :)

On Mon, May 13, 2013 at 9:09 PM, Manuel Klimek <klimek at google.com> wrote:

> On Mon, May 13, 2013 at 8:01 PM, Shuxin Yang <shuxin.llvm at gmail.com>wrote:
>
>>  Just FYI,   You never know if the APFloat allocate a memory or not after
>> construction, meaning
>>  you may start by calling APFloat with single-precision semantics (which
>> place its
>> significant bit in place, in stead of calling allocation); later on, some
>> arithmetic may extend
>> the precision for some reasons,  then it has to allocate some memory, and
>> it never reclaim the memory until destructor.
>>
>> Depending on when u register the call-back function, you are still able
>> to leak memory.
>>
>
> As far as I can tell only the last value (the compile time constant that
> is placed in the AST) is placement-newed, and thus needs to be freed in the
> end. Everything during evaluation is using normal allocation, and the last
> value is then copied into a placement newed object. Then we call
> needsCleanup on that object to see whether we'll need to store a pointer to
> the object to call the destructor on later or not. For most objects this is
> not necessary, thus it saves lots of memory.
>
> Cheers,
> /Manuel
>
>
>>
>>
>> On 5/13/13 10:49 AM, Shuxin Yang wrote:
>>
>>
>> On 5/13/13 10:37 AM, Manuel Klimek wrote:
>>
>> On Mon, May 13, 2013 at 6:26 PM, Shuxin Yang <shuxin.llvm at gmail.com>wrote:
>>
>>>
>>> On 5/13/13 10:21 AM, Manuel Klimek wrote:
>>>
>>> +richard smith, who proposed to implement it this way
>>>
>>> On Mon, May 13, 2013 at 6:08 PM, Shuxin Yang <shuxin.llvm at gmail.com>wrote:
>>>
>>>>  I'm afraid this change is going to open a big can of worms. You are
>>>> essentially promote
>>>> private member function to be public. Such practice should be generally
>>>> avoided even for
>>>> the local/private class which is used in small scope, let alone these
>>>> fundamental structures
>>>> which are extensively used in the compiler.
>>>>
>>>> > object needs the destructor called after its memory was freed
>>>>
>>>>  If the block of memory containing APFloat/APInt is already freed, how
>>>> do you know
>>>> the state of these objects are still valid? Depending on the
>>>> implementation, free-operation
>>>> might clobber the memory with some special value for debugging purpose.
>>>> If it were safe
>>>> to call needsCleanup(), why not directly call the destructor? Is
>>>> needsCleanup() sufficient
>>>> and/or necessary condition for calling the destructor? How do we keep
>>>> them in sync in the
>>>> future?
>>>>
>>>
>>>  APFloat/APInt is placement new'ed in the code in question, and thus we
>>> need to call the destructors of any objects that do memory allocation
>>> themselves. We can save those destructor calls otherwise.
>>>
>>>  I understand that if you place a non-plain-old-data in a union, you
>>> have to construct it via placement new,
>>> and explicitly destruct it.  I come across similar problem when I
>>> implement unsafe fadd/fsub optimization
>>> half year ago.
>>>
>>> My questions are :
>>>   - why do you need to call function xyz() before calling the
>>> destructor.
>>>   - if the memory is already freed, why do you know it is safe to call
>>> syz().
>>>      The object may not in valid state.
>>>
>>
>>  Ah, now I understand the question :)
>>
>>  So, in clang, the lifetime of the placement new'ed APFloat/APInt is
>> basically coupled to the buffer of where they are placement new'ed into,
>> which is very different from where the objects are created. Thus, when the
>> objects are placement-new'ed, clang registers cleanup callbacks with that
>> structure (which basically just calls the destructor for those objects
>> before the underlying placement-new-buffer is deallocated). Since there
>> might be a ton of APFloat/APInt values constructed inside a C++ AST, we
>> want to minimize the number of registered callbacks (all of them use memory
>> and precious runtime).
>>
>> I'm sorry, I don't know the internal of clang.
>> How about using a enum is keep track of the state of the buffer (if it
>> contains a valid APFloat/APInt/whatever), and so
>> you just need to register one call-back like this:
>>
>>  my-call-back(the-buffer) {
>>      switch(buffer-state)
>>      case is_APFloat:
>>           ((APFloat*)&buffer-state->data)::~APFloat();
>>           buffer-state = invalid;
>>      case is_APint:
>>            ...
>>      case is_plain-old-data:
>>           do nothing
>>  }
>>
>>   To that end, we call needsCleanup() in order to check whether we need
>> to register a "destructor callback" that needs to be called before throwing
>> away the memory. (see the clang review CL I pointed to)
>>
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130514/3405151c/attachment.html>