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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:197
+// Set LiveRoot flag on entries matching the given value name.
+static void setLiveRoot(ModuleSummaryIndex &Index, std::string Name) {
+  auto SummaryList =
----------------
`s/std::string/StringRef/`


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:305
               /* NoRename */ true,
+              /* LiveRoot */ true,
               /* HasInlineAsmMaybeReferencingInternal */ false,
----------------
Not sure about this after reading the comment above, `Also, any values used but not defined within module level asm should be listed on the llvm.used or llvm.compiler.used global and marked as referenced from there`.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:792
   bool HasInlineAsmMaybeReferencingInternal = RawFlags & 0x4;
-  return GlobalValueSummary::GVFlags(Linkage, NoRename,
+  bool LiveRoot = (RawFlags & 0x8) || Version < 3;
+  return GlobalValueSummary::GVFlags(Linkage, NoRename, LiveRoot,
----------------
One line comment for the `Version < 3` (it makes sense to me now, but I'm not sure I'll see it immediately if I have to touch this in a few months).


================
Comment at: lib/LTO/LTO.cpp:603
             ExportList->second.count(GUID)) ||
-           ExportedGUIDs.count(GUID);
+           (!DeadSymbols.count(GUID) && ExportedGUIDs.count(GUID));
   };
----------------
tejohnson wrote:
> If a symbol is in ExportedGUIDs, wouldn't it necessarily also be in GUIDPreservedSymbols and therefore not dead (if it is VisibleToRegularObj the partition would also have been set to External). So do we even need the ExportedGUIDs check here? And if not, do we need to compute it?
What is the answer to your question here?


================
Comment at: lib/LTO/LTO.cpp:847
+  for (auto &Res : GlobalResolutions) {
+    if (Res.second.VisibleToRegularObj && !Res.second.IRName.empty())
+      GUIDPreservedSymbols.insert(GlobalValue::getGUID(Res.second.IRName));
----------------
What if `Res.second.VisibleToRegularObj && Res.second.IRName.empty()`? Should we assert instead?


https://reviews.llvm.org/D23488