[PATCH] D18702: [WIP/LTO] Improve internalize decisions

[Davide Italiano via llvm-commits]

davide created this revision.
davide added reviewers: rafael, pcc.
davide added a subscriber: llvm-commits.
davide set the repository for this revision to rL LLVM.
Herald added a subscriber: joker.eph.

The existing algorithm was wrong because it internalized symbols it didn't need to. 
I'm bootstrapping right now to see if it actually fixes the failure I hit. Consider this a WIP/FYI patch, I'll upload a definitive version later today. Refer to the code for comments/details.

Repository:
  rL LLVM

http://reviews.llvm.org/D18702

Files:
  ELF/LTO.cpp
  test/ELF/lto/internalize-exportdyn.ll

Index: test/ELF/lto/internalize-exportdyn.ll
===================================================================
--- test/ELF/lto/internalize-exportdyn.ll
+++ test/ELF/lto/internalize-exportdyn.ll
@@ -10,10 +10,10 @@
   ret void
 }
 
-define hidden void @foo() {
+define void @foo() {
   ret void
 }
 
 ; Check that _start and foo are not internalized.
 ; CHECK: define void @_start()
-; CHECK: define hidden void @foo()
+; CHECK: define void @foo()
Index: ELF/LTO.cpp
===================================================================
--- ELF/LTO.cpp
+++ ELF/LTO.cpp
@@ -71,6 +71,18 @@
     saveBCFile(M, ".lto.opt.bc");
 }
 
+static bool isVisible(SymbolBody *Body) {
+  if (Body->MustBeInDynSym)
+    return true;
+  if (Config->Shared || Config->ExportDynamic) {
+    auto Visibility = Body->getVisibility();
+    if ((Visibility == llvm::ELF::STV_DEFAULT ||
+        Visibility == llvm::ELF::STV_PROTECTED))
+      return true;
+  }
+  return false;
+}
+
 void BitcodeCompiler::add(BitcodeFile &F) {
   std::unique_ptr<IRObjectFile> Obj =
       check(IRObjectFile::create(F.MB, Context));
@@ -114,14 +126,23 @@
     // we imported the symbols and satisfied undefined references
     // to it. We can't just change linkage here because otherwise
     // the IRMover will just rename the symbol.
-    // Shared libraries need to be handled slightly differently.
-    // For now, let's be conservative and just never internalize
-    // symbols when creating a shared library.
-    if (!Config->Shared && !Config->ExportDynamic && !B->isUsedInRegularObj())
+   if (B->isUsedInRegularObj()) {
+      // Case 1: The symbol is referenced from the outside. We can't
+      // internalize this symbol.
+      Keep.push_back(GV);
+    } else if (isVisible(B)) {
+      // Case 2: The symbol is exported to the symbol table. Maybe we
+      // can internalize the symbol. So we put in a different set
+      // and we defer the decision after the mover did its job.
+      Keep.push_back(GV);
+    }
+    else {
+      // Case 3: The symbol is neither referenced or visible.
+      // We can internalize it.
       if (!Used.count(GV))
         InternalizedSyms.insert(GV->getName());
-
-    Keep.push_back(GV);
+      Keep.push_back(GV);
+    }
   }
 
   Mover.move(Obj->takeModule(), Keep,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D18702.52395.patch
Type: text/x-patch
Size: 2289 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160401/43ae14af/attachment-0001.bin>