[PATCH] D28978: [ThinLTO] Add an auto-hide feature

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D28978#661111, @pcc wrote:

> In https://reviews.llvm.org/D28978#661105, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D28978#661102, @pcc wrote:
> >
> > > > To get this, I believe right now we need one mode field in the symbol resolution API (example: `unsigned VisibleOutsideDSO : 1`) to distinguish  between the two first cases,
> > >
> > > As we discussed on IRC, I don't think that's necessary. The linker can resolve this and decide on its own whether to add the symbol to the dynamic symbol table.
> >
> >
> > You're right, we can't do any optimization or any better codegen based on this information.
> >
> > I still think we should refine the partitioning detection, since only the first two cases would be merged.
> >
> >   External 
> >         | > Outside of LTO
> >         | > PartitionLTO
> >               | > RegularLTO
> >               | > PartitionThinLTO
> >                     | > Individual ThinLTO Module
> >
> >
> > What is introduced that we don't have today is a `PartitionLTO` and a `PartitionThinLTO`. This avoids going up to External immediately when a symbol is used in multiples individual ThinLTO modules, or when it is used across Regular and ThinLTO.
> >
> > Right new we have to reconstruct this information in ThinLTO because we're losing it when constructing the partitioning.
>
>
> I'm not sure I understand how `PartitionLTO` would be different from `!IsVisibleToRegularObj`, or `PartitionThinLTO` from `Partition == External && !VisibleOutsideThinLTO`.


It is not different, that's the point: VisibleOutsideThinLTO was added after the fact because the partition were losing the information. I think it was not a good addition and fixing the partitions instead was the way to go,  `Partition == External && !VisibleOutsideThinLTO` is not something I'd like to read anywhere.
.


https://reviews.llvm.org/D28978