[PATCH] D16440: [ThinLTO] Link in only necessary DICompileUnit operands

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: lib/Linker/IRMover.cpp:1386
@@ +1385,3 @@
+  // that is needed, and we should only invoke this for needed composite types.
+  assert(RMI != RetainMap.end());
+  // If we already created a new composite type declaration, use it.
----------------
I'm not sure this is true. Here's a type you might have to import that is not in the retained types list:

  namespace {
  struct foo { };
  void f(foo) { }
  }
  struct bar { };
  void f(bar) { }
  void f() {
    f(foo());
    f(bar());
  }

The 'foo' type is not in the retained types list, but it may need to be imported if 'f(foo)' is imported, yes?

================
Comment at: lib/Linker/IRMover.cpp:1398
@@ +1397,3 @@
+      Composite->getFile(), Composite->getLine(), NewScope, NewBaseType,
+      Composite->getSizeInBits(), Composite->getAlignInBits(),
+      Composite->getOffsetInBits(), Composite->getFlags() | DINode::FlagFwdDecl,
----------------
Pretty sure you don't need the base type, size, alignment, and offset in a declaration - check equivalent declarations that are generated by clang? (check that the declarations this code creates look like natural declarations in the original source language, etc)

================
Comment at: lib/Linker/IRMover.cpp:1411
@@ +1410,3 @@
+Metadata *
+IRLinker::importType(Metadata *Ty,
+                     DenseMap<DICompositeType *, DICompositeType *> &RetainMap,
----------------
This name seems a bit misleading. If I'm reading the code correctly, this is used for any type's scope context - which might not be a type at all. (it could be a namespace, for example) - and I assume the non-type cases come out in the "map the type metadata normally" part?

================
Comment at: lib/Linker/IRMover.cpp:1427
@@ +1426,3 @@
+
+void IRLinker::linkImportedCompileUnit() {
+  NamedMDNode *SrcCompileUnits = SrcM.getNamedMetadata("llvm.dbg.cu");
----------------
Name seems a bit off - this links in potentially multiple compile units, not just a single one.

================
Comment at: lib/Linker/IRMover.cpp:1444
@@ +1443,3 @@
+    std::vector<DICompositeType *> RetainWorklist;
+    if (!IsMetadataLinkingPostpass) {
+      for (DIType *Ty : CU->getRetainedTypes()) {
----------------
What's postpass metadata linking? (what's the alternative?)

================
Comment at: lib/Linker/IRMover.cpp:1445
@@ +1444,3 @@
+    if (!IsMetadataLinkingPostpass) {
+      for (DIType *Ty : CU->getRetainedTypes()) {
+        // For non-postpass metadata linking, any metadata referenced
----------------
I'm not really following why we need to iterate the retained types list - wouldn't we import everything starting from the subprograms we needed to import, not the retained types list?

================
Comment at: lib/Linker/IRMover.cpp:1534
@@ +1533,3 @@
+    stripNullSubprograms(NewCU);
+    DestCompileUnits->addOperand(NewCU);
+  }
----------------
What if nothing was imported from this CU?

It seems to me we'd want to start at the imported functions, follow their DISubprograms, and import that way, rather than walking all the CUs and subprograms - maybe?


http://reviews.llvm.org/D16440