[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/ThinLTOCodeGenerator.cpp:727
             ExportList->second.count(GUID)) ||
-           GUIDPreservedSymbols.count(GUID);
+           (!DeadSymbols.count(GUID) && GUIDPreservedSymbols.count(GUID));
   };
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > Actually, in ThinLTOCodeGenerator checks here and elsewhere it looks like this test can just be !DeadSymbols.count(GUID) rather than (!DeadSymbols.count(GUID) && GUIDPreservedSymbols.count(GUID)). Here the same GUIDPreservedSymbols set was passed computeDeadSymbols, so they would already be excluded from the DeadSymbol set. ThinLTOCodeGenerator does not seem to distinguish between references outside of ThinLTO vs cross-references between ThinLTO modules, which in fact means that the dead analysis will be conservative here...
> I think the expectation is that GUIDPreservedSymbols is only supposed to contain symbols visible outside of the ThinLTO partition.
> 
> From the header:
> 
> ```
>   /// Set of symbols that need to be preserved outside of the set of bitcode
>   /// files.
>   StringSet<> PreservedSymbols;
> ```
> 
> Even though in the current impl. file:
> 
> ```
> void ThinLTOCodeGenerator::crossReferenceSymbol(StringRef Name) {
>   // FIXME: At the moment, we don't take advantage of this extra information,
>   // we're conservatively considering cross-references as preserved.
>   //  CrossReferencedSymbols.insert(Name);
>   PreservedSymbols.insert(Name);
> }
> ```
> Even though in the current impl. file:

Right, that's what I was referring to. Any reason crossReferenceSymbol() doesn't add to a different set (the commented out CrossReferencedSymbols?) - although the isExported checks here and elsewhere in ThinLTOCodeGenerator would need to change if that happened. Maybe better to just switch this to the new LTO API?


https://reviews.llvm.org/D23488