[PATCH] D17321: DIEData, DIEWriter: introduce and begin migration.

[David Blaikie via llvm-commits]

dblaikie added a comment.

The hashing stuff does seem a bit worrying - not sure how that'll end up looking.

The odd/even abbrev numbering seems a bit awkward - can we just build the new infrastructure closer to the old stuff, so they can pick numbers more closely? (have you done any size analysis of this change - I realize the numbering of DIEs is probably a small component of DWARF size, but this change sounds like it'd increase DWARF size a little?)

What do you have in mind for DIEs that are incomplete for most of DWARF emission? (user defined types (classes and structs), and subprogram/function descriptions are like this - as we were discussing in previous threads) The abbreviation won't be known until the end of DWARF emission, so it seems we'll want to keep around a representation of the abbreviation that's more like what we have today. It seems like this transition approach might result in more churn than would be necessary, prehaps? But maybe not - just wonedring/thinking about it.

I haven't looked at everything in detail - more review tomorrow, etc.


================
Comment at: include/llvm/CodeGen/DIEData.h:152
@@ +151,3 @@
+      return attr1(Attr, dwarf::DW_FORM_data1, Data);
+    else if (Data < (1ull << 16))
+      return attr2(Attr, dwarf::DW_FORM_data2, Data, BE);
----------------
else-after-return

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:685
@@ -684,1 +684,3 @@
 
+void DwarfUnit::string(DIEWriter &W, dwarf::Attribute Attr, StringRef Str) {
+  DwarfStringPool::EntryRef Entry = DU->getStringPool().getEntry(*Asm, Str);
----------------
Probably best not to name a function the same as a common library type. I think functions that use string as a verb usually end up named "stringify" or the like.. but that might not be what's being described here, not sure yet.

================
Comment at: unittests/CodeGen/DIEDataTest.cpp:84
@@ +83,3 @@
+  {
+    DIEWriter W{Abbrevs, Data, dwarf::DW_TAG_subprogram, false};
+    W.attr4(dwarf::DW_AT_type, dwarf::DW_FORM_ref4, 1, false);
----------------
Use () rather than {}, this doesn't look like it should be an aggregate/implicit ctor (but we haven't really bothered to  put 'explicit' on all the ctors that became implicit in C++11 with aggregate init)


http://reviews.llvm.org/D17321