Linker: Move special casing for available_externally in IRMover to clients. NFCI.

[pcc]

Summary

The goal is to simplify the semantic model for clients of IRMover.

[mehdi_amini]

llvm/lib/LTO/LTO.cpp

481

Do we have a test that shows that:

1. if we first import an available_externally, then a later linkonce_odr (and/or strong) would overwrite?
2. if we first import a linkonce_odr (and/or strong), a later available_externally wouldn't overwrite?

llvm/lib/Linker/IRMover.cpp

875

No test changed, that's annoying that we're not covering this case (independently of the new LTO API). Any idea how to test this? (along with the removal below as well)

[mehdi_amini]

LGTM otherwise.

[tejohnson]

llvm/lib/Linker/LinkModules.cpp

398

I guess removing this is required to make it NFC. Is it required for correctness too? I don't think so but want to make sure my understanding is right. Is it better to continue not mapping these in here (I think they get mapped later if referenced, right?) and not bother making this NFC?

[pcc]

• Add test
• A change is required in a different place to make it NFC

[pcc]

llvm/lib/LTO/LTO.cpp

481

I've added tests for both of those things, as well as a standalone test for the available_externally functionality (previously only a gold plugin test was covering this).

llvm/lib/Linker/IRMover.cpp

875

The only direct users of IRMover are LinkModules and LTO. I guess we could write a unit test for IRMover perhaps? Doesn't seem worth it for this one change though.

llvm/lib/Linker/LinkModules.cpp

398

Actually this change was needed for us to link available_externally at all (with only the change to shouldLink we would hit the 'return false;' here and never link any available_externally globals). But taking a closer look I found that we were linking too much, even unreferenced available_externally globals (which was a functional change). I've uploaded a new change that causes available_externally to be handled like linkonce and only linked if referenced.

[mehdi_amini]

Thanks for the tests! Still LGTM.

llvm/lib/Linker/IRMover.cpp

875

I wouldn't suggest it :)

[mehdi_amini]

llvm/test/Linker/available_externally_a.ll

7

Actually why isn't llvm-link including the available_externally?

[pcc]

llvm/test/Linker/available_externally_a.ll

7

Because it is unreferenced, see discussion with Teresa in LinkModules.cpp.

[mehdi_amini]

llvm/test/Linker/available_externally_a.ll

7

Oh right :)

llvm/lib/LTO/LTO.cpp

for (const InputFile::Symbol &Sym :
        make_range(InputFile::symbol_iterator(SymTab.symbols().begin(), SymTab,
                                              nullptr),
                   InputFile::symbol_iterator(SymTab.symbols().end(), SymTab,
                                              nullptr))) {
     assert(ResI != ResE);
     SymbolResolution Res = *ResI++;
     addSymbolToGlobalRes(Used, Sym, Res, 0);
 
+    if (Sym.isGV()) {
+      GlobalValue *GV = Sym.getGV();
+      if (Res.Prevailing) {
         if (Sym.getFlags() & object::BasicSymbolRef::SF_Undefined)
           continue;
-    if (Res.Prevailing && Sym.isGV()) {
-      GlobalValue *GV = Sym.getGV();
         Keep.push_back(GV);
         switch (GV->getLinkage()) {
         default:
           break;
         case GlobalValue::LinkOnceAnyLinkage:
           GV->setLinkage(GlobalValue::WeakAnyLinkage);
           break;
         case GlobalValue::LinkOnceODRLinkage:
           GV->setLinkage(GlobalValue::WeakODRLinkage);
           break;
         }
+      } else if (GV->hasAvailableExternallyLinkage()) {
+        // We can link available_externally symbols even if they are
+        // non-prevailing.
+        GlobalValue *CombinedGV =
+            RegularLTO.CombinedModule->getNamedValue(GV->getName());
+        if (!CombinedGV || CombinedGV->isDeclaration())
+          Keep.push_back(GV);
+      }
     }
     // Common resolution: collect the maximum size/alignment over all commons.
     // We also record if we see an instance of a common as prevailing, so that
     // if none is prevailing we can ignore it later.
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       // FIXME: We should figure out what to do about commons defined by asm.
       // For now they aren't reported correctly by ModuleSymbolTable.
       auto &CommonRes = RegularLTO.Commons[Sym.getGV()->getName()];

llvm/lib/Linker/IRMover.cpp

 
 bool IRLinker::shouldLink(GlobalValue *DGV, GlobalValue &SGV) {
   if (ValuesToLink.count(&SGV) || SGV.hasLocalLinkage())
     return true;
 
   if (DGV && !DGV->isDeclarationForLinker())
     return false;
 
-  if (SGV.hasAvailableExternallyLinkage())
-    return true;
-
   if (SGV.isDeclaration() || DoneLinkingBodies)
     return false;
 
   // Callback to the client to give a chance to lazily add the Global to the
   // list of value to link.
   bool LazilyAdded = false;
   AddLazyFor(SGV, [this, &LazilyAdded](GlobalValue &GV) {
     maybeAdd(&GV);

llvm/lib/Linker/LinkModules.cpp

bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
   if (GV.hasAppendingLinkage() && isPerformingImport())
     return false;
 
   if (isPerformingImport()) {
     if (!doImportAsDefinition(&GV))
       return false;
   } else if (!DGV && !shouldOverrideFromSrc() &&
              (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||
               GV.hasAvailableExternallyLinkage()))
     return false;
 
   if (GV.isDeclaration())
     return false;
 
   if (const Comdat *SC = GV.getComdat()) {
     bool LinkFromSrc;
     Comdat::SelectionKind SK;

 void ModuleLinker::addLazyFor(GlobalValue &GV, const IRMover::ValueAdder &Add) {
   if (!shouldLinkReferencedLinkOnce())
     // For ThinLTO we don't import more than what was required.
     // The client has to guarantee that the linkonce will be availabe at link
     // time (by promoting it to weak for instance).
     return;
 
   // Add these to the internalize list
-  if (!GV.hasLinkOnceLinkage() && !shouldLinkOnlyNeeded())
+  if (!GV.hasLinkOnceLinkage() && !GV.hasAvailableExternallyLinkage() &&
+      !shouldLinkOnlyNeeded())
     return;
 
   if (shouldInternalizeLinkedSymbols())
     Internalize.insert(GV.getName());
   Add(GV);
 
   const Comdat *SC = GV.getComdat();
   if (!SC)

llvm/test/LTO/Resolution/X86/Inputs/link-odr-availextern-ae.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define available_externally i32 @f() {
+  ret i32 2
+}

llvm/test/LTO/Resolution/X86/link-odr-availextern.ll

+; Tests for correct behavior for non-prevailing resolutions in cases involving
+; *_odr and available_externally linkages.
+
+; RUN: llvm-as %s -o %t1
+; RUN: llvm-as %S/Inputs/link-odr-availextern-ae.ll -o %t2ae
+
+; RUN: llvm-lto2 -o %t3 %t1 %t2ae -r %t1,f,p -r %t2ae,f, -save-temps
+; RUN: llvm-dis < %t3.0.0.preopt.bc -o - | FileCheck --check-prefix=PREVAILING %s
+
+; RUN: llvm-lto2 -o %t3 %t2ae %t1 -r %t1,f,p -r %t2ae,f, -save-temps
+; RUN: llvm-dis < %t3.0.0.preopt.bc -o - | FileCheck --check-prefix=PREVAILING %s
+
+; RUN: llvm-lto2 -o %t3 %t2ae -r %t2ae,f, -save-temps
+; RUN: llvm-dis < %t3.0.0.preopt.bc -o - | FileCheck --check-prefix=NONPREVAILING %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; PREVAILING: define weak_odr i32 @f()
+; PREVAILING-NEXT: ret i32 1
+; NONPREVAILING: define available_externally i32 @f()
+; NONPREVAILING-NEXT: ret i32 2
+define linkonce_odr i32 @f() {
+  ret i32 1
+}

llvm/test/Linker/available_externally_a.ll

 ; RUN: llvm-link %s %p/available_externally_b.ll -S -o - | FileCheck %s
+; RUN: llvm-link %s -S -o - | FileCheck --check-prefix=AE-ONLY %s
 
 @foo = available_externally unnamed_addr constant i32 0
 
 ; CHECK: @foo = hidden unnamed_addr constant i32 0
+; AE-ONLY-NOT: @foo