[ThinLTO/gold] Change preempted def to avail extern linkage when possible

[tejohnson]

Summary

Similar to the approach being discussed for libLTO in D18346,
optimize the ThinLTO backend compilations and object files by converting
preempted weak and linkonce symbols to available_externally when
possible. This ensures that they are dropped and don't unnecessarily
show up in the object file.

[tejohnson]

As discussed in D18346, alias/aliasee cannot be available_externally.

Augmented test with alias case.

[• rafael]

tools/gold/gold-plugin.cpp

779

It is OK to have available_externally to a comdat member, no?

I mean, it is equivalent to a declaration, and we can have a declaration to a comdat member.

[• rafael]

trying to get phab to send an email.

[tejohnson]

tools/gold/gold-plugin.cpp

779

You can't have a declaration or available_externally in a comdat. From Verifier.cpp:

if (GV.isDeclarationForLinker())
  Assert(!GV.hasComdat(), "Declaration may not be in a Comdat!", &GV);

[tejohnson]

• Handle comdat members

[• rafael]

test/tools/gold/X86/thinlto_linkonceresolution.ll

28

It just be just extern, not extern_weak, no?

tools/gold/gold-plugin.cpp

785

If the alias is preempted, you can turn it to a global variable declaration.

841

You can always set the comdat to null, so I would drop this if.

[tejohnson]

Thanks for the review. I'm going to abandon these however and instead use the support I just refactored out of ThinLTOCodeGenerator in r270698, which will do the same using a callback to get info about prevailing symbols.

test/tools/gold/X86/thinlto_linkonceresolution.ll

28

Yes, this line was changed in r266752 after Mehdi changed this in the ModuleLinker, but I hadn't rebased for awhile.

tools/gold/gold-plugin.cpp

785

True, but that would prevent inlining. Probably better to keep the inlining opportunity intact.

test/tools/gold/X86/Inputs/thinlto_linkonceresolution.ll

 target triple = "x86_64-unknown-linux-gnu"
+@analias = linkonce_odr alias void (), void ()* @analiasee
+define linkonce_odr hidden void @analiasee() {
+  ret void
+}
 define linkonce_odr hidden void @f() {
   ret void
 }
+define linkonce hidden void @h() {
+  ret void
+}

test/tools/gold/X86/Inputs/thinlto_weakresolution.ll

+target triple = "x86_64-unknown-linux-gnu"
+define weak_odr hidden void @f() {
+  ret void
+}
+define weak hidden void @h() {
+  ret void
+}

test/tools/gold/X86/thinlto_linkonceresolution.ll

 ; RUN: llvm-as -module-summary %s -o %t.o
 ; RUN: llvm-as -module-summary %p/Inputs/thinlto_linkonceresolution.ll -o %t2.o
 
 ; Ensure the plugin ensures that for ThinLTO the prevailing copy of a
 ; linkonce symbol is changed to weak to ensure it is not eliminated.
 ; Note that gold picks the first copy of f() as the prevailing one,
 ; so listing %t2.o first is sufficient to ensure that this copy is
 ; preempted. Also, set the import-instr-limit to 0 to prevent f() from
 ; being imported from %t2.o which hides the problem.
 ; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:     --plugin-opt=thinlto \
 ; RUN:     --plugin-opt=-import-instr-limit=0 \
 ; RUN:     --plugin-opt=save-temps \
+; RUN:     --plugin-opt=O0 \
 ; RUN:     -o %t3.o %t2.o %t.o
 ; RUN: llvm-nm %t3.o | FileCheck %s
 ; RUN: llvm-dis %t.o.opt.bc -o - | FileCheck --check-prefix=OPT %s
 ; RUN: llvm-dis %t2.o.opt.bc -o - | FileCheck --check-prefix=OPT2 %s
 
-; Ensure that f() is defined in resulting object file, and also
-; confirm the weak linkage directly in the saved opt bitcode files.
+; Ensure that f() and h() are defined in resulting object file.
 ; CHECK-NOT: U f
-; OPT: declare extern_weak hidden void @f()
-; OPT2: define weak_odr hidden void @f()
+; CHECK-NOT: U h
+
+; The non-prevailing copy can be changed to available_externally in the
+; linkonce_odr case, but must remain linkonce in the non-ODR case (which
+; is eventually changed to an extern_weak declaration).
+; OPT-DAG: define available_externally hidden void @f()
+; OPT-DAG: declare extern_weak hidden void @h()
+; Cannot convert alias or aliasee to available_externally (not a valid
+; linkage type for alias, aliasee must be real definition for linker).
+; OPT-DAG: @analias = extern_weak global void ()
+; OPT-DAG: declare extern_weak hidden void @analiasee()
+
+; The prevailing copy must be converted to weak so that it is kept.
+; OPT2-DAG: define weak_odr hidden void @f()
+; OPT2-DAG: define weak hidden void @h()
+; OPT2-DAG: @analias = weak_odr alias void (), void ()* @analiasee
+; OPT2-DAG: define weak_odr hidden void @analiasee()
 
 target triple = "x86_64-unknown-linux-gnu"
 define i32 @g() {
   call void @f()
+  call void @h()
+  call void @analias()
+  call void @analiasee()
   ret i32 0
 }
+@analias = linkonce_odr alias void (), void ()* @analiasee
+define linkonce_odr hidden void @analiasee() {
+  ret void
+}
 define linkonce_odr hidden void @f() {
   ret void
 }
+define linkonce hidden void @h() {
+  ret void
+}

test/tools/gold/X86/thinlto_weakresolution.ll

+; RUN: llvm-as -module-summary %s -o %t.o
+; RUN: llvm-as -module-summary %p/Inputs/thinlto_weakresolution.ll -o %t2.o
+
+; Check that for ThinLTO the plugin converts the preempted copy of a
+; weak symbol to available_externally if it is weak_odr, or leaves it
+; alone in the non-ODR case.
+; Note that gold picks the first copy of f() as the prevailing one,
+; so listing %t2.o first is sufficient to ensure that this copy is
+; preempted. Also, set the import-instr-limit to 0 to prevent any effects
+; from importing.
+; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:     --plugin-opt=thinlto \
+; RUN:     --plugin-opt=-import-instr-limit=0 \
+; RUN:     --plugin-opt=save-temps \
+; RUN:     --plugin-opt=O0 \
+; RUN:     -o %t3.o %t2.o %t.o
+; RUN: llvm-nm %t3.o | FileCheck %s
+; RUN: llvm-dis %t.o.opt.bc -o - | FileCheck --check-prefix=OPT %s
+; RUN: llvm-dis %t2.o.opt.bc -o - | FileCheck --check-prefix=OPT2 %s
+
+; Ensure that f() and h() are defined in resulting object file.
+; CHECK-NOT: U f
+; CHECK-NOT: U h
+
+; The non-prevailing copy can be changed to available_externally in the
+; weak_odr case, but must remain weak in the non-ODR case (which
+; is eventually changed to an extern_weak declaration).
+; OPT: define available_externally hidden void @f()
+; OPT: declare extern_weak hidden void @h()
+
+; The prevailing copy remains weak so that it is kept.
+; OPT2: define weak_odr hidden void @f()
+; OPT2: define weak hidden void @h()
+
+target triple = "x86_64-unknown-linux-gnu"
+define i32 @g() {
+  call void @f()
+  call void @h()
+  ret i32 0
+}
+define weak_odr hidden void @f() {
+  ret void
+}
+define weak hidden void @h() {
+  ret void
+}

tools/gold/gold-plugin.cpp

getModuleForFile(LLVMContext &Context, claimed_file &F, const void *View,
   Module &M = Obj.getModule();
 
   M.materializeMetadata();
   UpgradeDebugInfo(M);
 
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
 
+  DenseSet<GlobalValue *> Aliasees;
+  DenseSet<GlobalValue *> PossibleAvailableExternally;
+
   unsigned SymNum = 0;
   for (auto &ObjSym : Obj.symbols()) {
     GlobalValue *GV = Obj.getSymbolGV(ObjSym.getRawDataRefImpl());
     if (GV && GV->hasAppendingLinkage())
       Keep.push_back(GV);
 
+    if (auto *GA = dyn_cast<GlobalAlias>(GV))
+      Aliasees.insert(GA->getBaseObject());
+
     if (shouldSkip(ObjSym.getFlags()))
       continue;
     ld_plugin_symbol &Sym = F.syms[SymNum];
     ++SymNum;
 
     ld_plugin_symbol_resolution Resolution =
         (ld_plugin_symbol_resolution)Sym.resolution;
 
for (auto &ObjSym : Obj.symbols()) {
 
     switch (Resolution) {
     case LDPR_UNKNOWN:
       llvm_unreachable("Unexpected resolution");
 
     case LDPR_RESOLVED_IR:
     case LDPR_RESOLVED_EXEC:
     case LDPR_RESOLVED_DYN:
+      break;
+
     case LDPR_PREEMPTED_IR:
     case LDPR_PREEMPTED_REG:
+      // For ThinLTO we can change the linkage type of preempted
+      // symbols to available_externally so that they can be dropped during
+      // the ThinLTO backend compile step. This is unnecessary for full LTO
+      // where all copies will be merged into a single one. Note that we
+      // already ensure the prevailing copy gets weak linkage in the ThinLTO
+      // so that it isn't eliminated if unreferenced in its module.
+      if (options::thinlto &&
+          // Can't do this for WeakAny/LinkOnceAny since they may be overridden
+          // and are thus ineligible for optimizations such as being inlined.
+          // FIXME: In the non-ODR case drop the definitions.
+          (GV->hasWeakODRLinkage() || GV->hasLinkOnceODRLinkage()) &&
+          // Alias can't be available_externally.
+          // FIXME: Turn the alias into a global and duplicate the definition.
+          !isa<GlobalAlias>(GV))
+        // Convert later if this is not found to be an aliasee.
+        PossibleAvailableExternally.insert(GV);
       break;
 
     case LDPR_UNDEF:
       if (!GV->isDeclarationForLinker())
         assert(GV->hasComdat());
       break;
 
     case LDPR_PREVAILING_DEF_IRONLY: {
case LDPR_PREVAILING_DEF_IRONLY_EXP: {
       Keep.push_back(GV);
       break;
     }
     }
 
     freeSymName(Sym);
   }
 
+  // Convert to available_externally when not found to be an aliasee.
+  // FIXME: Turn the alias into a global and duplicate the definition.
+  for (auto *GV : PossibleAvailableExternally) {
+    if (Aliasees.count(GV))
+      continue;
+
+    GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
+
+    if (!GV->hasComdat())
+      continue;
+    if (auto *GO = dyn_cast<GlobalObject>(GV))
+      GO->setComdat(nullptr);
+  }
+
   return Obj.takeModule();
 }
 
 static void saveBCFile(StringRef Path, Module &M) {
   std::error_code EC;
   raw_fd_ostream OS(Path, EC, sys::fs::OpenFlags::F_None);
   if (EC)
     message(LDPL_FATAL, "Failed to write the output file.");