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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
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));
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > What if `Res.second.VisibleToRegularObj && Res.second.IRName.empty()`? Should we assert instead?
> > Looking at the code in addSymbolToGlobalRes, IRName is set when the GV is marked as prevailing. So I guess "Res.second.VisibleToRegularObj && Res.second.IRName.empty()" would mean a symbol that is visible to a regular object but not the prevailing copy. In that case, it shouldn't need to be preserved.
> The fact that you had to look at `addSymbolToGlobalRes` to provide the explanation is a good indication that a comment would be welcome here.
Ok will do


================
Comment at: lib/LTO/LTO.cpp:335
       (GlobalRes.Partition != GlobalResolution::Unknown &&
-       GlobalRes.Partition != Partition))
+       GlobalRes.Partition != Partition)) {
     GlobalRes.Partition = GlobalResolution::External;
----------------
mehdi_amini wrote:
> Can you add a comment that explains the test (and both branches)
Will do


================
Comment at: lib/LTO/LTO.cpp:341
+  GlobalRes.VisibleToRegularLTOPartition |=
+      (Partition == GlobalResolution::RegularLTO);
 }
----------------
mehdi_amini wrote:
> It is not clear to me how the `VisibleToRegularLTOPartition` is computed here, it seems that for partition to be 0 (RegularLTO) the symbol has to be used or defined in LTO but not used or defined in ThinLTO?
This will be called for each partition it is referenced from. Partition 0 is always regular LTO, ThinLTO modules are partitions 1 and higher. So if this is ever called for Partition 0 then we have a reference in regular LTO. See the callsites in add*LTO


https://reviews.llvm.org/D23488