[PATCH] Introduce needsCleanup() for APFloat and APInt

[Manuel Klimek]

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/20130513/c39fc58f/attachment.html>