<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 10, 2015 at 12:14 PM, Frederic Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">================<br>
Comment at: include/llvm/CodeGen/DIE.h:135<br>
@@ -110,3 +134,3 @@<br>
<br>
-class DIE {<br>
+class DIE : public ilist_node<DIE> {<br>
protected:<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> This might be able to be private inheritance? (just handle the DIEs by unique_ptr and then pass them into addChild)<br>
><br>
> Hmm, I suppose it'd need to friend the ilist instantiation & maybe other things, for this to be private inheritance?<br>
</span>Could try it, though I don't think I've seen that pattern anywhere else.<br></blockquote><div><br>Fair enough *shrug* up to you, just a thought.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: include/llvm/CodeGen/DIE.h:157<br>
@@ -132,3 +156,3 @@<br>
// be more convoluted than beneficial.<br>
- std::vector<std::unique_ptr<DIE>> Children;<br>
+ ilist<DIE> Children;<br>
<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> I've a bit of an aversion to intrusive lists - so I wonder if you'd be willing to revisit the issues in the comment above (including the issue about why list<DIE> was insufficient - picking up from where I left off to see if we could reasonably rely on post-insertion pointer validity without too much convolution?).<br>
><br>
> Actually why move to ilist? These objects will never have their dtors run, right? So you could just switch from vector<unique_ptr<DIE>> to vector<DIE*> - everything's non-owning now, since their memory is managed by the BumpPtrAllocator entirely?<br>
</span>If we use list<DIE>, we'd need to use a std custom allocator that wraps the BumpPtrAllocator which would complicate things quite a bit. I could try it though.</blockquote><div><br>Right right.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The whole point of the ilist here is that allocating a DIE allocates everything that's needed for the list bookkeeping. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Re: vector<DIE*>, you'd leak the vector storage if you do not call the destructor on every DIE.<br></blockquote><div><br>Ah, right.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: include/llvm/CodeGen/DIE.h:220<br>
@@ -184,3 +219,3 @@<br>
Child->Parent = this;<br>
Children.push_back(std::move(Child));<br>
}<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> If we're going to go with ilist, could we improve/overload ilist's push_back to take unique_ptr for clarity?<br>
</span>I can try that.<br>
<span class=""><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:67<br>
@@ -66,2 +66,3 @@<br>
UnitTag == dwarf::DW_TAG_type_unit);<br>
+ UnitDie = new (DIEValueAllocator) DIE(UnitTag);<br>
DIEIntegerOne = new (DIEValueAllocator) DIEInteger(1);<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Doesn't seem like it'd be too important to allocate the unit die with the DIEValueAllocator, does it?<br>
><br>
> I guess if you don't, then it can end up in the list of DIEs to deallocate, and ends up double-deleted? Again, this would be the sort of subtlety it might be best to avoid.<br>
</span>It's not important from a performance standpoint, but if I find the logic easier to follow if the rule is "allocate everything DIE related on the BumpPtrAllocator". If we allow this on to be a an exception to this rule, we must then handle its destruction specifically (as you point out, removing it from the DIEsToDelete list, but also making sure it drops its children and does not delete them).<br></blockquote><div><br>Yeah, I'm really a bit concerned about the subtlety of the DIE type and how easily it could be misused as it stands.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.h:116<br>
@@ -113,1 +115,3 @@<br>
+ // called so that the SmallVector's memory isn't leaked.<br>
+ std::vector<DIE*> DIEsToDelete;<br>
<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> This seems a bit subtle - having the SmallVector's dtors not be run unless they happen to go over the limit & reallocate. Could we use the existing BumpPtrAllocator as the allocator to the SmallVectors? Or something similar. (perhaps we discussed this, it's been a while)<br>
</span>SmallVector don't accept configurable allocators AFAICS. I tried going with a std::vector with a custom allocator that wrapped a mix of recycliing + BumpPtrAllocator, but it didn't perform well.<br>
<br>
I agree it's subtle, but it's the best tradeoff I could find.<br>
<span class=""><br>
================<br>
Comment at: unittests/CodeGen/DIEHashTest.cpp:626<br>
@@ -625,3 +625,3 @@<br>
DIEEntry PITy(*PITyDIE);<br>
- auto PI = make_unique<DIE>(dwarf::DW_TAG_member);<br>
+ DIE *PI = new DIE(dwarf::DW_TAG_member);<br>
DIEString PIStr(&One, "PI");<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Again, bit subtle - in the real use case this is non-owning because the parent's dtor is never run, etc, etc. But in the test case it is owning because the parent's dtor is run?<br>
><br>
> Hmm, that raises another question - once a DIE is added to the "DIEs to delete" list, what stops it from deleting its children DIEs? (since it's a member std::ilist in there) possibly leading to excess deletes (inefficiencies you're trying to remove) or double/triple/multiple deletes if children are in the "DIEs to delete" list too?<br>
</span>This approach basically gives you the choice of ownership. If you wanna use something like a BumpPtrAllocator, then you use the DIEsToDelete trick to avoid leaking memory.<br>
<br>
If you wanna use a more 'traditional' memory management scheme, it's also possible like in this test.</blockquote><div><br>I'm not sure this is a feature - it seems more like complexity that would easily lead to mis-use...<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The DIEs are still owned by their parents through the ilist of children, which means calling the root destructor will destroy everything.<br>
<br>
This is handled in the BumpPtrAllocator by preventing the DIEs from the DIEsToDelete array to delete their children (look for clearAndLeakNodesUnsafely() in ~DwarfUnit). Yeah, subtle again...<br></blockquote><div><br>Right.<br><br>Maybe it'd be best to move to a manually managed linked list, so it's clear that this is all non-owning? Or at least have ~DIE's dtor explicitly release ownership of the elements of the ilist (but even this I find to be questionable - if these things aren't owned (& they're not - because they might be in the DIESToDelete list) then we shouldn't put them in a container that has ownership... )<br><br>- David</div></div></div></div>