This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Add Visibility bits to GlobalValueSummary::GVFlags
ClosedPublic

Authored by MaskRay on Dec 8 2020, 5:36 PM.

Details

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.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 8 2020, 5:36 PM
MaskRay requested review of this revision.Dec 8 2020, 5:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 5:36 PM
MaskRay updated this revision to Diff 310413.Dec 8 2020, 8:19 PM
MaskRay edited the summary of this revision. (Show Details)

Rebase. Fix a bug

tejohnson added inline comments.Dec 9 2020, 1:59 PM
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 updated this revision to Diff 310707.Dec 9 2020, 4:14 PM
MaskRay marked 3 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

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 added a subscriber: lhames.Dec 9 2020, 4:25 PM
MaskRay added inline comments.
llvm/lib/Transforms/IPO/FunctionImport.cpp
1027

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

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 updated this revision to Diff 318787.Jan 23 2021, 1:14 PM
MaskRay edited the summary of this revision. (Show Details)

Address comments:

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

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 updated this revision to Diff 319618.Jan 27 2021, 10:33 AM
MaskRay marked 4 inline comments as done.

Improve comments

This revision is now accepted and ready to land.Jan 27 2021, 10:37 AM
This revision was landed with ongoing or failed builds.Jan 27 2021, 10:44 AM
This revision was automatically updated to reflect the committed changes.