[PATCH] D19103: Add an internalization step to the ThinLTOCodeGenerator

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:325
@@ +324,3 @@
+// the current module. This is inspired from IRObjectFile.
+static void CollectAsmUndefinedRefs(Module &TheModule,
+                                    StringSet<> &AsmUndefinedRefs) {
----------------
It seems there is already some support in libLTO for detecting these, see the call to LTOModule::addAsmGlobalSymbolUndef(). Can that be leveraged here? What is the difference between what is being detected by these two cases?

If it needs to use the IRObject method, can that functionality be refactored into a method in libObject that is callable from here? It seems like it could return the AsmSymbols vector and you could just iterate that here looking for a Key that is BasicSymbolRef::SF_Undefined in the returned pair, and is !TheModule.getNamedValue(Key). Using a refactored common routine would make it easier to do similar optimizations in the gold and lld cases as well.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:394
@@ +393,3 @@
+
+//
+static DenseSet<const GlobalValue *> computePreservedSymbolsForModule(
----------------
Missing comment

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:402
@@ +401,3 @@
+    // supply anything to preserve.
+    return PreservedGV;
+
----------------
I guess the assumption is that there is nothing to export if there is nothing to preserve? Should there be an assertion here that ExportList is empty?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:422
@@ +421,3 @@
+
+// Run internalization on \p TheModule
+static void
----------------
doxygen style /// comment (here and with other added functions)

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:436
@@ +435,3 @@
+  // symbols referenced from asm
+  UpdateCompilerUsed(TheModule, TM, AsmUndefinedRefs);
+
----------------
Why not add these to the PreservedGV set?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:689
@@ +688,3 @@
+  // conservatively considering cross-references as preserved.
+  //  CrossReferencedSymbols.insert(Name);
+  PreservedSymbols.insert(Name);
----------------
Should this be a FIXME?


http://reviews.llvm.org/D19103