[ThinLTO] Add Visibility bits to GlobalValueSummary::GVFlags

[MaskRay]

Summary

Imported functions and variable get the visibility from the module supplying the
definition. However, non-imported definitions do not get the visibility from
(ELF) the most constraining visibility among all modules (Mach-O) the visibility
of the prevailing definition.

This patch

• adds visibility bits to GlobalValueSummary::GVFlags
• computes the result visibility and propagates it to all definitions

Protected/hidden can imply dso_local which can enable some optimizations (this
is stronger than GVFlags::DSOLocal because the implied dso_local can be
leveraged for ELF -shared while default visibility dso_local has to be cleared
for ELF -shared).

Note: we don't have summaries for declarations, so for ELF if a declaration has
the most constraining visibility, the result visibility may not be that one.

[MaskRay]

Rebase. Fix a bug

[tejohnson]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

981

It's getting a little hard to follow what is in what bit position. Would you be able to document the current layout here and in the BitcodeWriter?

Also, I guess we don't need to bump the version since old bitcode will just end up with default visibility, right?

llvm/lib/IR/ModuleSummaryIndex.cpp

45

Shouldn't this be taking the visibility from the prevailing copy? I'm not sure what the valid rule is - visibility from prevailing copy if it is more constraining, or the most constraining across all copies (as is done here currently).

The patch description talks about prevailing and non-prevailing defs, but the application in FunctionImport.cpp talks about all definitions.

llvm/lib/Transforms/IPO/FunctionImport.cpp

1002–1003

Update description since it is now fixing up visibility as well.

llvm/test/ThinLTO/X86/visibility.ll

33 ↗

(On Diff #310413)

I thought this patch should fix that?

37 ↗

(On Diff #310413)

Ditto

[MaskRay]

Set the visility for the prevailing definition as well.
Restrict to ELF (tested by visibility-macho.s; Mach-O uses the visibility from the prevailing definition; this patch does not intend to optimize it)
Improve comments

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

981

Yes. The visibility bits are from the previously unused bits.

llvm/lib/IR/ModuleSummaryIndex.cpp

45

Mach-O takes the visibility from the prevailing definition. I've renamed the function to getELFVisibility to make it clear.

The ELF reference is this paragraph on http://www.sco.com/developers/gabi/latest/ch4.symtab.html "Second, if any reference to or definition of a name is a symbol with a non-default visibility attribute, the visibility attribute must be propagated to the resolving symbol in the linked object. If different visibility attributes are specified for distinct references to or definitions of a symbol, the most constraining visibility attribute must be propagated to the resolving symbol in the linked object. The attributes, ordered from least to most constraining, are: STV_PROTECTED, STV_HIDDEN and STV_INTERNAL."

In short, every undefined and defined symbol takes part in the visibility computation and the most constraining one wins.

llvm/lib/Transforms/IPO/FunctionImport.cpp

1002–1003

I'll drop the comment and update the header comment instead.

llvm/test/ThinLTO/X86/visibility.ll

33 ↗

(On Diff #310413)

This is still not fixed because IIUC declarations are not in the summary list so their contributions cannot be leveraged.

37 ↗

(On Diff #310413)

This is still not fixed because IIUC declarations are not in the summary list so their contributions cannot be leveraged.

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1027

@lhames This is what I discussed in the discord room:)

[tejohnson]

From your patch description:

Imported functions and variables get the visibility from the prevailing module.

This isn't quite accurate, since we don't guarantee that will import the prevailing copy. E.g. in the linkonce_odr case, in most cases we pick the first copy which is the one the linker marks as prevailing in practice, but we can in certain cases import a different copy. I would just say "Imported functions and variable get the visibility from the module supplying the definition". Then the next sentence should instead be something like "However, non-imported definitions do not get the visibility from the most constraining visibility among all modules."

llvm/lib/IR/ModuleSummaryIndex.cpp

45

Ok got it. With the current patch, the new visibility flags are always set during the thin link in the summaries as if we were doing ELF, but then applied in the backend only if it does turn out to be ELF. But it seems like we could do a similar optimization for MachO, if we knew here from the linker which scheme to apply (e.g. take visibility from prevailing copy rather than use most constraining). The caller of this function does know which is the prevailing copy, via the isPrevailing callback, so we could implement that if we knew that was the requirement. Even if the MachO scheme isn't implemented right now, it seems better to me to have the linker provide info via the LTO Config as to where to take the visibility from (e.g. an enum there specifying different schemes), and then we can just blindly apply the visibility in the ThinLTO backends. This simplifies future extension to other binary formats.

llvm/test/ThinLTO/X86/visibility.ll

33 ↗

(On Diff #310413)

Got it, in this case we can't apply to a declaration, because we don't have a summary for it in that module after the thin link. Suggest making this comment something like:
;; Currently the visibility is not propagated onto an unimported function, because we don't have summaries for declarations.

37 ↗

(On Diff #310413)

And in this case, we don't have a summary for a declaration on which we can communicate the visibility to other modules. However, this is an interesting case because we had hidden on the declaration that was in this module, and it looks like when we imported the protected def from the other module the less constraining visibility "overwrote" the hidden visibility that was on the declaration here. Presumably this happened in the IRLinker? I guess that is implementing something like MachO semantics?

Suggest making this comment something like:

;; This can be hidden, but we cannot communicate the declaration's visibility to other modules because
;; declarations don't have summaries, and the IRLinker overrides it when importing the protected def.

[MaskRay]

Address comments:

• Improve comments.
• Add Config::{FromPrevailing,ELF} to represent two schemes.
• Drop IsELF from FunctionImport.cpp

[tejohnson]

Looks pretty good, just a few small suggestions.

llvm/include/llvm/LTO/Config.h

82

Add comment on why this default.

llvm/lib/LTO/LTO.cpp

326

Minor efficiency suggestion - only invoke getELFVisibility under Config::ELF, so we don't walk through all the summary entries (e.g. potentially long for linkonce_odr) unnecessarily.

llvm/lib/Transforms/IPO/FunctionImport.cpp

1020

The comments here about ELF and extending to other binary formats should be moved since this code no longer is looking at format. Maybe to getELFVisibility and to the declaration of Config::VisScheme?

[MaskRay]

Improve comments

[tejohnson]

lgtm