[PATCH] Introduce needsCleanup() for APFloat and APInt

[Manuel Klimek]

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).
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)

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130513/e0091385/attachment.html>