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

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:402
@@ +401,3 @@
+    // supply anything to preserve.
+    return PreservedGV;
+
----------------
tejohnson wrote:
> 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?
No this is completely independent: if the client didn't supply anything to preserve, we don't internalize at all. Changed the comment.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:422
@@ +421,3 @@
+
+// Run internalization on \p TheModule
+static void
----------------
tejohnson wrote:
> doxygen style /// comment (here and with other added functions)
I think it is relevant only for public API. These are static function so they won't show up in doxygen: http://llvm.org/doxygen/ThinLTOCodeGenerator_8cpp.html

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:436
@@ +435,3 @@
+  // symbols referenced from asm
+  UpdateCompilerUsed(TheModule, TM, AsmUndefinedRefs);
+
----------------
tejohnson wrote:
> Why not add these to the PreservedGV set?
PreservedGV drives the internalizer, this is possibly driving other places in LLVM I think, like GlobalDCE for example.

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


http://reviews.llvm.org/D19103