[PATCH] D21080: [ThinLTO/gold] Enable summary-based internalization

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:534
@@ +533,3 @@
+        // recorded in the index using the original name.
+        const auto &GS = DefinedGlobals.find(GlobalValue::getGUID(OrigName));
+        assert(GS != DefinedGlobals.end());
----------------
mehdi_amini wrote:
> Note: we plan to change the alias representation to https://llvm.org/bugs/show_bug.cgi?id=27866 ; which I believe will make this issue obsolete. It may even remove the need for the aliases representation in the summary (not sure what are the implications about comdats).
> 
> Also, this seems like a bug fix that could be exposed with llvm-lto, and thus be split out of this patch?
Ok, I can add a FIXME to reevaluate this after that bug is fixed.

Let me see if I can provoke this with llvm-lto and split it out.

================
Comment at: tools/gold/gold-plugin.cpp:1326
@@ +1325,3 @@
+  StringSet<> Internalize;
+  StringSet<> CrossReferenced;
+
----------------
mehdi_amini wrote:
> Could these contain directly GUIDs?
Yes, will switch.

================
Comment at: tools/gold/gold-plugin.cpp:1367
@@ -1329,1 +1366,3 @@
+  for (auto &S : CrossReferenced)
+    Internalize.erase(S.first());
 
----------------
mehdi_amini wrote:
> Isn't there an API to subtract a set from another one?
Not that I could find in either StringSet or DenseSet.


http://reviews.llvm.org/D21080