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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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;
----------------
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?


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1009
 
 /// Fixup prevailing symbol linkages in \p TheModule based on summary analysis.
 void llvm::thinLTOResolvePrevailingInModule(
----------------
Update description since it is now fixing up visibility as well.


================
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()
----------------
I thought this patch should fix that?


================
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
----------------
Ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92900