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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:978
   auto Linkage = GlobalValue::LinkageTypes(RawFlags & 0xF); // 4 bits
+  auto Visibility = GlobalValue::VisibilityTypes((RawFlags >> 8) & 3); // 2 bits
   RawFlags = RawFlags >> 4;
----------------
tejohnson wrote:
> 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?
Yes. The visibility bits are from the previously unused bits.


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1009
 
 /// Fixup prevailing symbol linkages in \p TheModule based on summary analysis.
 void llvm::thinLTOResolvePrevailingInModule(
----------------
tejohnson wrote:
> Update description since it is now fixing up visibility as well.
I'll drop the comment and update the header comment instead.


================
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()
----------------
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.


================
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
----------------
tejohnson wrote:
> Ditto
This is still not fixed because IIUC declarations are not in the summary list so their contributions cannot be leveraged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92900