[PATCH] D92900: [ThinLTO] Add Visibility bits to GlobalValueSummary::GVFlags

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

>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."



================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:45
+  bool HasProtected = false;
+  for (const auto &S : make_pointee_range(getSummaryList())) {
+    if (S.getVisibility() == GlobalValue::HiddenVisibility)
----------------
MaskRay wrote:
> tejohnson wrote:
> > 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.
> 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.
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.


================
Comment at: llvm/test/ThinLTO/X86/visibility.ll:33
 ; CHECK: declare hidden void @protected_def_weak_hidden_def()
 ;; Currently the visibility is not propagated for an unimported function.
 ; CHECK: declare extern_weak void @not_imported()
----------------
MaskRay wrote:
> tejohnson wrote:
> > I thought this patch should fix that?
> This is still not fixed because IIUC declarations are not in the summary list so their contributions cannot be leveraged.
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.


================
Comment at: llvm/test/ThinLTO/X86/visibility.ll:37
 ; CHECK: define available_externally hidden void @hidden_def_weak_ref() !thinlto_src_module !0
 ;; This can be hidden.
 ; CHECK: define available_externally protected void @protected_def_hidden_ref() !thinlto_src_module !0
----------------
MaskRay wrote:
> tejohnson wrote:
> > Ditto
> This is still not fixed because IIUC declarations are not in the summary list so their contributions cannot be leveraged.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92900/new/

https://reviews.llvm.org/D92900