[PATCH] D23488: ThinLTO: add early "dead-stripping" on the Index

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:98
@@ -95,1 +97,3 @@
+    StringMap<FunctionImporter::ExportSetTy> &ExportLists,
+    const DenseSet<GlobalValue::GUID> *DeadSymbols = nullptr);
 
----------------
Please also add this handling to LTO.cpp so that the optimization is available for all client linkers. As a bonus, then DeadSymbols doesn't need ot be optional. For LTO.cpp, the set of preserved GUIDs is currently computed just below the call to ComputeCrossModuleImport as ExportedGUIDs, and can simply be moved up so that it can be passed to computeDeadSymbols.

Alternatively, do we want to instead pass in the set of preserved GUIDs to ComputeCrossModuleImport and invoke computeDeadSymbols there, or do we anticipate that there will be other uses outside of ComptueCrossModuleImport? If not, then I think it is cleaner to do the dead symbol computation there.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:529
@@ +528,3 @@
+  // Compute "dead" symbols, we don't want to import/export these!
+  auto DeadSymbols = computeDeadSymbols(Index, GUIDPreservedSymbols);
+
----------------
There are now a bunch of duplicated copies of the same sequence in this source file: collectDefinedGVSummariesPerModule, computeGUIDPreservedSymbols, computeDeadSymbols, ComputeCrossModuleImport. Probably time to refactor this sequence into a helper? Ok as a follow-on patch if you prefer though.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:467
@@ +466,3 @@
+
+    // FIXME: we should only make the prevailing copy live here
+    for (auto &Summary : It->second) {
----------------
Do you mean we should only make the prevailing copy's references live here?

This should be pretty straightforward, just pull out the detection of prevailing copies from resolveWeakForLinkerInIndex and pass in a callback. For LTO.cpp, the isPrevailing lambda definition is just below the call to ComputeCrossModuleImport and could be moved up. Ok as a follow on patch though.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:473
@@ +472,3 @@
+          DEBUG(dbgs() << "Marking live (ref): " << (uint64_t)RefGUID << " "
+                       << (int64_t)RefGUID << "\n");
+          Worklist.push_back(RefGUID);
----------------
Why emit the referenced GUID twice here and in the below loop, rather than just once with the default type (which is already uint64_t so first cast unnecessary).

================
Comment at: test/ThinLTO/X86/deadstrip.ll:15
@@ +14,3 @@
+
+; The final binary should not contain any of the dead function, only main is expected
+; CHECK-NM-NOT: bar
----------------
Might want to add that bar* are dead because they should have been inlined into main.


https://reviews.llvm.org/D23488