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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D23488#627220, @tejohnson wrote:

> In https://reviews.llvm.org/D23488#627205, @pcc wrote:
>
> > Also think about how this interacts with regular LTO. You probably also want anything referenced from the regular LTO module to be treated as a GC root.
>
>
> Good point - I think we would miss those right now with this patch.


I just realized I haven't addressed this yet. Looks like it requires adding a new flag into the SymbolResolution, since we can't currently distinguish between a value referenced between different ThinLTO partitions and the case where we have a reference between a ThinLTO partition and the regular LTO partition - in both cases the resulting GlobalResolution Partition will be External. I'll probably have to add a VisibleToRegularLTOPartition flag onto the SymbolResolution and set it when the Partition == External and we have seen a reference from Partition 0. I'll do that and add a test.



================
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 =
----------------
mehdi_amini wrote:
> `s/std::string/StringRef/`
Fixed


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:305
               /* NoRename */ true,
+              /* LiveRoot */ true,
               /* HasInlineAsmMaybeReferencingInternal */ false,
----------------
mehdi_amini wrote:
> 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`.
Note that part of the comment is referring to "values used but not defined" within the module level asm. Whereas we are creating a summary here for something defined within module level asm. To be conservatively correct these should be marked live.


================
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,
----------------
mehdi_amini wrote:
> 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).
Done.


================
Comment at: lib/LTO/LTO.cpp:603
             ExportList->second.count(GUID)) ||
-           ExportedGUIDs.count(GUID);
+           (!DeadSymbols.count(GUID) && ExportedGUIDs.count(GUID));
   };
----------------
mehdi_amini wrote:
> 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?
I realized when revisiting the patch that my logic was flawed. It can be exported and dead (in fact, that's one thing we are specifically trying to catch with this patch). Additionally, it can be live (not in DeadSymbols) and not be exported. So your original check was correct. We want to treat as exported when internalizing if it was a) original exported to another module and b) it isn't dead per the index based deadness analysis.


================
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:
> 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.


https://reviews.llvm.org/D23488