<div dir="ltr"><div style>Richard, anything to add? :)</div><div><br></div>On Mon, May 13, 2013 at 9:09 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="im">On Mon, May 13, 2013 at 8:01 PM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank">shuxin.llvm@gmail.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
Just FYI, You never know if the APFloat allocate a memory or not
after construction, meaning <br>
you may start by calling APFloat with single-precision semantics
(which place its <br>
significant bit in place, in stead of calling allocation); later on,
some arithmetic may extend <br>
the precision for some reasons, then it has to allocate some
memory, and<br>
it never reclaim the memory until destructor. <br>
<br>
Depending on when u register the call-back function, you are still
able to leak memory. <br></div></blockquote><div><br></div></div><div>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.</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div><div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div><div>
<br>
<br>
<div>On 5/13/13 10:49 AM, Shuxin Yang wrote:<br>
</div>
<blockquote type="cite">
<br>
<div>On 5/13/13 10:37 AM, Manuel Klimek
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">On Mon, May 13, 2013 at 6:26 PM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank">shuxin.llvm@gmail.com</a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div> <br>
<div>On 5/13/13 10:21 AM, Manuel Klimek wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">+richard smith, who proposed to
implement it this way
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, May 13, 2013
at 6:08 PM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank">shuxin.llvm@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> I'm
afraid this change is going to open a
big can of worms. You are essentially
promote <br>
private member function to be public.
Such practice should be generally
avoided even for <br>
the local/private class which is used in
small scope, let alone these fundamental
structures <br>
which are extensively used in the
compiler. <br>
<div> <br>
> object needs the destructor
called after its memory was freed <br>
<br>
</div>
If the block of memory containing
APFloat/APInt is already freed, how do
you know <br>
the state of these objects are still
valid? Depending on the implementation,
free-operation <br>
might clobber the memory with some
special value for debugging purpose. If
it were safe<br>
to call needsCleanup(), why not directly
call the destructor? Is needsCleanup()
sufficient <br>
and/or necessary condition for calling
the destructor? How do we keep them in
sync in the <br>
future?<br>
</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
</div>
I understand that if you place a non-plain-old-data in
a union, you have to construct it via placement new, <br>
and explicitly destruct it. I come across similar
problem when I implement unsafe fadd/fsub optimization
<br>
half year ago. <br>
<br>
My questions are : <br>
- why do you need to call function xyz() before
calling the destructor. <br>
- if the memory is already freed, why do you know it
is safe to call syz(). <br>
The object may not in valid state.<br>
</div>
</blockquote>
<div><br>
</div>
<div>Ah, now I understand the question :)</div>
<div><br>
</div>
<div>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).</div>
</div>
</div>
</div>
</blockquote>
I'm sorry, I don't know the internal of clang.<br>
How about using a enum is keep track of the state of the buffer
(if it contains a valid APFloat/APInt/whatever), and so <br>
you just need to register one call-back like this:<br>
<br>
my-call-back(the-buffer) {<br>
switch(buffer-state) <br>
case is_APFloat:<br>
((APFloat*)&buffer-state->data)::~APFloat();<br>
buffer-state = invalid;<br>
case is_APint:<br>
...<br>
case is_plain-old-data:<br>
do nothing<br>
}<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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)</div>
<div><br>
</div>
<br>
</div>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>