[PATCH] D18252: Drop comdats from the dst module if they are not selected

[Teresa Johnson via llvm-commits]

tejohnson added a subscriber: tejohnson.

================
Comment at: lib/Linker/LinkModules.cpp:105
@@ -104,1 +104,3 @@
 
+  void dropReplacedComdat(GlobalValue &GV,
+                          const DenseSet<const Comdat *> &ReplacedDstComdats);
----------------
Needs doxygen comment

================
Comment at: lib/Linker/LinkModules.cpp:477
@@ +476,3 @@
+    PointerType &Ty = *cast<PointerType>(Alias.getType());
+    GlobalValue::LinkageTypes L = Alias.getLinkage();
+    GlobalValue *Declaration =
----------------
nit: just invoke Alias.getLinkage() in below constructor call and remove local L.

Also is it even correct to use the original linkage as-is in all cases? Should it be ExternalLinkage?

Finally, what if this is ThinLTO and the alias was defined in the original module we are importing into? Won't we have an undef at link time? Ditto for full LTO - where would this new declaration be defined? I'm not even sure what the right behavior is in that case.

================
Comment at: lib/Linker/LinkModules.cpp:479
@@ +478,3 @@
+    GlobalValue *Declaration =
+        new GlobalVariable(M, Ty.getElementType(), /*isConstant*/ false, L,
+                           /*Initializer*/ nullptr);
----------------
What if this was a function alias not variable alias?

================
Comment at: lib/Linker/LinkModules.cpp:504
@@ +503,3 @@
+    bool LinkFromSource = P.second.second;
+    if (!LinkFromSource)
+      continue;
----------------
Why not combine this into the above loop? I.e. when the ComdatsChosen array is initialized for C, you already have its LinkFromSrc. And that step is done exactly once for each comdat due to the earlier check that continues if C is already in ComdatsChosen.

================
Comment at: lib/Linker/LinkModules.cpp:509
@@ +508,3 @@
+    StringRef ComdatName = C->getName();
+    Module::ComdatSymTabType::iterator DstCI = ComdatSymTab.find(ComdatName);
+    if (DstCI == ComdatSymTab.end())
----------------
Just invoke C->getName() and remove ComdatName local?

================
Comment at: test/Linker/comdat-rm-dst.ll:12
@@ +11,3 @@
+
+; RM-NOT: ailas
+ at alias = alias i32, i32* @foo
----------------
s/ailas/alias/ to get desired check


http://reviews.llvm.org/D18252