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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/LTO/LTO.h:389
+    /// Keep track if the symbol was referenced by the regular LTO partition.
+    bool VisibleToRegularLTOPartition = false;
+
----------------
pcc wrote:
> mehdi_amini wrote:
> > pcc wrote:
> > > Don't you just need one flag: VisibleOutsideThinLTO (or something).
> > I think one flag should be enough, but shouldn't we have the same flag for LTO internalization?
> > 
> Wouldn't that be redundant with Partition == 0?
You're right, we only need one flag (forgot that the VisibleToRegularObj flag was added to GlobalResolutions for this patch). I will combine them as suggested.


================
Comment at: lib/LTO/LTO.cpp:341
+  GlobalRes.VisibleToRegularLTOPartition |=
+      (Partition == GlobalResolution::RegularLTO);
 }
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > tejohnson wrote:
> > > 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
> > I misread the assignment as `=` instead of `|=`.
> Also technically to be only "visible" to the RegularLTO partition, it should be referenced from LTO but not defined there with a prevailing resolution.
I'm using the term "visible" loosely here - if it is defined in the regular LTO partition it is essentially also visible there.


https://reviews.llvm.org/D23488