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

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 22 11:16:24 PST 2016


mehdi_amini 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));
----------------
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.


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


================
Comment at: lib/LTO/LTO.cpp:341
+  GlobalRes.VisibleToRegularLTOPartition |=
+      (Partition == GlobalResolution::RegularLTO);
 }
----------------
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?


https://reviews.llvm.org/D23488





More information about the llvm-commits mailing list