This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Add an auto-hide feature
ClosedPublic

Authored by mehdi_amini on Jan 20 2017, 11:00 PM.

Details

Summary

when a symbol is not exported outside of the
DSO, it is can be hidden. Usually we try to internalize
as much as possible, but it is not always possible, for
instance a symbol can be referenced outside of the LTO
unit, or there can be cross-module reference in ThinLTO.

I'm still testing this patch, it may a bit rough around
the edges right now.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini created this revision.Jan 20 2017, 11:00 PM
mehdi_amini added inline comments.Jan 27 2017, 10:45 AM
llvm/include/llvm/LTO/LTO.h
60 ↗(On Diff #85233)

This is a good starting point to understand this patch.

tejohnson added inline comments.Jan 30 2017, 2:12 PM
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
980 ↗(On Diff #85233)

Move this up and << 2 (consistent with LiveRoot and other bool flag setup)

llvm/lib/LTO/LTO.cpp
910 ↗(On Diff #85233)

Add TODO about distinguishing which are used in regular LTO partition vs outside LTO/IR.

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
900 ↗(On Diff #85233)

Consider renaming this method

llvm/test/ThinLTO/X86/weak_autohide.ll
4 ↗(On Diff #85233)

Unused

pcc edited edge metadata.Jan 30 2017, 2:20 PM

Does this need to be in the summary? I would have thought that the linker can use canBeOmittedFromSymbolTable() to implement its own auto-hiding logic. That is what ELF lld currently does.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
980 ↗(On Diff #85233)

It looks like we are setting this variable in a very confusing way. I'd move the linkage first and then shift NotEligibleToImport and LiveRoot by 4 more.

In D28978#660963, @pcc wrote:

Does this need to be in the summary?

The part that is in the summary is not set in the compiler phase, but after the think-link, in order to communicate the *result* of the auto-hide analysis to the parallel backends.

I would have thought that the linker can use canBeOmittedFromSymbolTable() to implement its own auto-hiding logic. That is what ELF lld currently does.

That's kind of what this patch is doing :)

Especially we're using Res.VisibleToRegularObj as an input information, and this is set in gold for example there:

case LDPR_PREVAILING_DEF_IRONLY_EXP:
  R.Prevailing = true;
  if (!Res.CanOmitFromDynSym)
    R.VisibleToRegularObj = true;
  break;

And the Res.CanOmitFromDynSym is set this way: Res.CanOmitFromDynSym &= Sym.canBeOmittedFromSymbolTable();.

I haven't checked Gold, but I think you get the point: this is only using the symbol resolution API and no "side-information" from the summary.

That said, I'm not totally happy by the resolution API right now, and especially how we derived the partition. We don't have all the information we want I believe.
I discussed it this morning with Teresa and what I think my current frustration is with the lack of precise enough lattice in the partitions resolution. I think we should be able to get this:

External 
      | > Regular Object
      | > Outside of the DSO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

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,

pcc added a comment.Jan 30 2017, 3:46 PM
In D28978#660963, @pcc wrote:

Does this need to be in the summary?

The part that is in the summary is not set in the compiler phase, but after the think-link, in order to communicate the *result* of the auto-hide analysis to the parallel backends.

Oh I see. So you're communicating VisibleToRegularObj and information about the partitioning to the backends. Seems reasonable to me.

I would have thought that the linker can use canBeOmittedFromSymbolTable() to implement its own auto-hiding logic. That is what ELF lld currently does.

That's kind of what this patch is doing :)

Especially we're using Res.VisibleToRegularObj as an input information, and this is set in gold for example there:

case LDPR_PREVAILING_DEF_IRONLY_EXP:
  R.Prevailing = true;
  if (!Res.CanOmitFromDynSym)
    R.VisibleToRegularObj = true;
  break;

And the Res.CanOmitFromDynSym is set this way: Res.CanOmitFromDynSym &= Sym.canBeOmittedFromSymbolTable();.

I haven't checked Gold, but I think you get the point: this is only using the symbol resolution API and no "side-information" from the summary.

That said, I'm not totally happy by the resolution API right now, and especially how we derived the partition. We don't have all the information we want I believe.
I discussed it this morning with Teresa and what I think my current frustration is with the lack of precise enough lattice in the partitions resolution. I think we should be able to get this:

External 
      | > Regular Object
      | > Outside of the DSO
      | > PartitionLTO
            | > RegularLTO
            | > PartitionThinLTO
                  | > Individual ThinLTO Module

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.

mehdi_amini marked an inline comment as done.Jan 30 2017, 3:55 PM
In 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.

pcc added a comment.Jan 30 2017, 4:11 PM
In 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.

In D28978#661111, @pcc wrote:
In 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.
.

pcc added a comment.Jan 30 2017, 4:42 PM
In D28978#661111, @pcc wrote:
In 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.

Agreed that we should rethink partitions, it would be nice to have a better way to write that query.

mehdi_amini marked 4 inline comments as done.

Address comments

mehdi_amini marked an inline comment as done.Feb 2 2017, 10:04 AM
mehdi_amini added inline comments.
llvm/lib/LTO/LTO.cpp
910 ↗(On Diff #85233)

After discussing with Peter, we believe this is something the native linker could do.

What is not done in the current patch but can help (a little) the Codegen is to propagate the FinalDefinitionInLinkageUnit, to every uses to skip the GOT.

tejohnson accepted this revision.Feb 2 2017, 10:17 AM

LGTM with comment nit below.

llvm/lib/LTO/LTO.cpp
910 ↗(On Diff #86847)

Nit: typos in comment:
s/hidding/hiding/
s/reference from/referenced from/

also maybe add "only" before "referenced from" to clarify.

This revision is now accepted and ready to land.Feb 2 2017, 10:17 AM
This revision was automatically updated to reflect the committed changes.

FYI: after talking more with @pcc and doing more tests: this is not meaningful and a strict subset of what the linker can do (and already does).

I revert the patch.

FYI I also just confirmed that this patch was causing linker errors in SPEC 450.soplex of the form:

/usr/bin/ld: error: hidden symbol '_ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_PKc' is not defined locally
/usr/bin/ld: error: hidden symbol '_ZSt4endlIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_' is not defined locally