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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Looks pretty good, just a few small suggestions.



================
Comment at: llvm/include/llvm/LTO/Config.h:82
 
+  VisScheme VisibilityScheme = FromPrevailing;
+
----------------
Add comment on why this default.


================
Comment at: llvm/lib/LTO/LTO.cpp:326
     const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols) {
+  GlobalValue::VisibilityTypes Visibility = VI.getELFVisibility();
   for (auto &S : VI.getSummaryList()) {
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1020
 
+    // For ELF, set visibility to the most constraining one from a definition.
+    // We don't track visibility from declarations so this may be more relaxed
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92900