[PATCH] [PATCH/RFC] Use a BumpAllocator for DIEs

[Frederic Riss]

================
Comment at: include/llvm/CodeGen/DIE.h:135
@@ -110,3 +134,3 @@
 
-class DIE {
+class DIE : public ilist_node<DIE> {
 protected:
----------------
dblaikie wrote:
> This might be able to be private inheritance? (just handle the DIEs by unique_ptr and then pass them into addChild)
> 
> Hmm, I suppose it'd need to friend the ilist instantiation & maybe other things, for this to be private inheritance?
Could try it, though I don't think I've seen that pattern anywhere else.

================
Comment at: include/llvm/CodeGen/DIE.h:157
@@ -132,3 +156,3 @@
   // be more convoluted than beneficial.
-  std::vector<std::unique_ptr<DIE>> Children;
+  ilist<DIE> Children;
 
----------------
dblaikie wrote:
> 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?).
> 
> 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?
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. The whole point of the ilist here is that allocating a DIE allocates everything that's needed for the list bookkeeping. 

Re: vector<DIE*>, you'd leak the vector storage if you do not call the destructor on every DIE.

================
Comment at: include/llvm/CodeGen/DIE.h:220
@@ -184,3 +219,3 @@
     Child->Parent = this;
     Children.push_back(std::move(Child));
   }
----------------
dblaikie wrote:
> If we're going to go with ilist, could we improve/overload ilist's push_back to take unique_ptr for clarity?
I can try that.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:67
@@ -66,2 +66,3 @@
          UnitTag == dwarf::DW_TAG_type_unit);
+  UnitDie = new (DIEValueAllocator) DIE(UnitTag);
   DIEIntegerOne = new (DIEValueAllocator) DIEInteger(1);
----------------
dblaikie wrote:
> Doesn't seem like it'd be too important to allocate the unit die with the DIEValueAllocator, does it?
> 
> 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.
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). 

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.h:116
@@ -113,1 +115,3 @@
+  // called so that the SmallVector's memory isn't leaked.
+  std::vector<DIE*> DIEsToDelete;
 
----------------
dblaikie wrote:
> 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)
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.

I agree it's subtle, but it's the best tradeoff I could find.

================
Comment at: unittests/CodeGen/DIEHashTest.cpp:626
@@ -625,3 +625,3 @@
   DIEEntry PITy(*PITyDIE);
-  auto PI = make_unique<DIE>(dwarf::DW_TAG_member);
+  DIE *PI = new DIE(dwarf::DW_TAG_member);
   DIEString PIStr(&One, "PI");
----------------
dblaikie wrote:
> 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?
> 
> 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?
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.

If you wanna use a more 'traditional' memory management scheme, it's also possible like in this test. The DIEs are still owned by their parents through the ilist of children, which means calling the root destructor will destroy everything.

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

http://reviews.llvm.org/D7072

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/