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

[David Blaikie]

================
Comment at: include/llvm/CodeGen/DIE.h:135
@@ -110,3 +134,3 @@
 
-class DIE {
+class DIE : public ilist_node<DIE> {
 protected:
----------------
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?

================
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;
 
----------------
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?

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

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

================
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;
 
----------------
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)

================
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");
----------------
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?

http://reviews.llvm.org/D7072

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