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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/LTO/LTO.cpp:484
+        if (!CombinedGV || CombinedGV->isDeclaration()) {
+          Keep.push_back(GV);
+          GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
----------------
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.


================
Comment at: llvm/test/LTO/Resolution/X86/link-availextern.ll:8
+; RUN: llvm-lto2 -o %t3 %t1 %t2 -r %t1,f, -r %t2,f, -save-temps
+; RUN: llvm-dis < %t3.0.0.preopt.bc -o - | FileCheck --check-prefix=SINGLE %s
+
----------------
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?


https://reviews.llvm.org/D29367