LTO: Link non-prevailing weak_odr, linkonce_odr or available_externally globals into the combined module with available_externally linkage.

[pcc]

Summary

These linkages mean that the ultimately prevailing symbol will have the same
semantics as any non-prevailing copy of the symbol, so we are free to ignore
the linker's resolution.

[tejohnson]

llvm/lib/LTO/LTO.cpp

478 ↗

(On Diff #86566)

Will the IRLinker not replace the existing available_externally definition with the prevailing copy since it is added above to Keep (ValuesToLink)?

484 ↗

(On Diff #86566)

CombinedGO should never have a Comdat at this point, since it was available_externally.

[pcc]

• Simplify

llvm/lib/LTO/LTO.cpp

478 ↗

(On Diff #86566)

Yes it will, removed this code.

484 ↗

(On Diff #86566)

Now moot

[pcc]

• Add one more test

[tejohnson]

llvm/lib/LTO/LTO.cpp

484 ↗

(On Diff #86569)

I don't think adding to the Keep is technically necessary because of http://llvm-cs.pcc.me.uk/lib/Linker/IRMover.cpp#873 (we always link in available_externally copies in the IRLinker). That's presumably why inglorion's D29366 worked which didn't do this, although I had to look it up because I was initially confused as to why it worked without adding to the Keep! IMO it is clearer to have the explicit add to Keep here and not rely on the IRLinker behavior - but in either case perhaps leave a comment (particularly if you end up removing it). If you end up removing it, then incoming available_externally should be removed.

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

8 ↗

(On Diff #86569)

Minor quibble with the names SINGLE and BOTH which seem a little confusing (e.g. we have "both" inputs here). Maybe NONPREVAILING and HAVEPREVAILING or something like that would be more descriptive?

[pcc]

• Address review comments

llvm/lib/LTO/LTO.cpp

484 ↗

(On Diff #86569)

I was also surprised to see that code in IRMover that always links available_externally, but I suspect that it may be the right place for it (linker-independent "optimisations" based on linkage).

If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.

I went ahead and changed this code to set linkage to available_externally unconditionally for non-prevailing. That revealed a bug of sorts in IRMover in that it would repeatedly link available_externally definitions from source modules if they were available, instead of being satisfied with the first one. I also fixed that in this patch. A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.

[pcc]

• Rename checks in test case

[tejohnson]

LGTM, but one suggestion on the IRMover change and a question about your FIXME.

llvm/lib/LTO/LTO.cpp

484 ↗

(On Diff #86569)

A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.

Ah - I should have noticed that, we have to do the same thing for ThinLTO (thinLTOResolveWeakForLinkerGUID).

If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.

Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.

llvm/lib/Linker/IRMover.cpp

873 ↗

(On Diff #86678)

The new check is almost identical to the one a couple lines up. So we should never hit this when DGV && !DGV->isDeclaration() unless it is available_externally - maybe add a comment to that effect?

[mehdi_amini]

llvm/lib/LTO/LTO.cpp

484 ↗

(On Diff #86569)

I was also surprised to see that code in IRMover that always links available_externally,

I personally *hate* this kind of behavior that exhibit magic hidden from the API. This makes the software component harder to user and reason about, and invariant unclear.

I mentioned it multiple times in the case of the IRMover during ThinLTO bring-up that I'd even consider this a layer violation in term of responsibility of the IRMover (which should be moving the IR we tell it to and nothing else, *deciding* what to move being the other layer.).

[pcc]

Depends on D29435

[pcc]

llvm/lib/LTO/LTO.cpp

484 ↗

(On Diff #86569)

Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.

Yes, we have the set ValuesToLink. I was imagining that IRLinker could link *_odr globals and change their linkage to available_externally if they were not members of ValuesToLink, similar to how it is already handling available_externally globals.

484 ↗

(On Diff #86569)

Fair point, it sounds like the consensus is in favour of doing everything explicitly in the LTO layer then.

I sent out D29435 which moves the available_externally special casing to clients and rebased this change on top of it.

[tejohnson]

llvm/lib/LTO/LTO.cpp

484 ↗

(On Diff #86569)

Yeah, making it explicit rather that relying on more IRLinker magic sounds better.

[pcc]

• Simplify test

llvm/trunk/lib/LTO/LTO.cpp

Error LTO::addRegularLTO(BitcodeModule BM, const SymbolResolution *&ResI,
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
 
   std::vector<GlobalValue *> Keep;
 
   for (GlobalVariable &GV : M.globals())
     if (GV.hasAppendingLinkage())
       Keep.push_back(&GV);
 
+  DenseSet<GlobalObject *> AliasedGlobals;
+  for (auto &GA : M.aliases())
+    if (GlobalObject *GO = GA.getBaseObject())
+      AliasedGlobals.insert(GO);
+
   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()) {
           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.
+      } else if (isa<GlobalObject>(GV) &&
+                 (GV->hasLinkOnceODRLinkage() || GV->hasWeakODRLinkage() ||
+                  GV->hasAvailableExternallyLinkage()) &&
+                 !AliasedGlobals.count(cast<GlobalObject>(GV))) {
+        // Either of the above three types of linkage indicates that the
+        // chosen prevailing symbol will have the same semantics as this copy of
+        // the symbol, so we can link it with available_externally linkage. We
+        // only need to do this if the symbol is undefined.
         GlobalValue *CombinedGV =
             RegularLTO.CombinedModule->getNamedValue(GV->getName());
-        if (!CombinedGV || CombinedGV->isDeclaration())
+        if (!CombinedGV || CombinedGV->isDeclaration()) {
           Keep.push_back(GV);
+          GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
+          cast<GlobalObject>(GV)->setComdat(nullptr);
+        }
       }
     }
     // 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.

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

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

llvm/trunk/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-as %S/Inputs/link-odr-availextern-odr.ll -o %t2odr
 
 ; 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 %t1 %t2odr -r %t1,f,p -r %t2odr,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 %t2odr %t1 -r %t1,f,p -r %t2odr,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
 
+; RUN: llvm-lto2 -o %t3 %t2odr -r %t2odr,f, -save-temps
+; RUN: llvm-dis < %t3.0.0.preopt.bc -o - | FileCheck --check-prefix=NONPREVAILING %s
+
+; RUN: llvm-lto2 -o %t3 %t2odr %t1 -r %t1,f, -r %t2odr,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
 }