[ThinLTO] Import symver directives for imported symbols (PR48214)

[hans]

Summary

When importing symbols from another module, also import any corresponding symver directives.

(I'm new to this part of the code, so please give me pointers on how I can turn this into something that's fit for landing.)

[tejohnson]

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

llvm/lib/Linker/IRMover.cpp

712

This function gets called once per global value being linked in, so it would redo the below code multiple times.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

490 ↗

(On Diff #308425)

leftover debugging output?

[hans]

In D92335#2424693, @tejohnson wrote:

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

Yes, that seems easier. Thanks!

llvm/lib/Linker/IRMover.cpp

712

Yes, to make this work I figured we'd pull the symver metadata into a data structure that could be queried quickly here. But this approach no longer applies anyway.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

490 ↗

(On Diff #308425)

Oops, yes. But these changes are no longer necessary anyway.

[hans]

Pulling in the symver directives in IRLinker::run() instead. (I didn't do a separate function since it's just a few lines, but happy to do that if you think it would be better.)

Please take another look.

[hans]

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

[tejohnson]

In D92335#2425679, @hans wrote:

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

I think that's fine to have that dependence.

Largely lgtm but suggestion for refactor and test below.

llvm/lib/Linker/IRMover.cpp

1473

I think it makes sense to handle this earlier where we handle inline asm when !IsPerformingImport, since they logically go together (i.e. for regular LTO merge them, for ThinLTO just pull in the symver). Maybe refactor out the two sets of handling into a single helper method that deals with inline asm.

llvm/test/ThinLTO/X86/import-symver.ll

9

Maybe show a second case where if we don't import bar we also don't import its symver? You could add a second case where we disable importing (add -import-instr-limit=0 to an invocation of llvm-lto -thinlto-action=import) and check the results.

[hans]

Adding test case where the symver doesn't get imported.

llvm/lib/Linker/IRMover.cpp

1473

I looked at that first, but at that point the actual linking/importing into DstM hasn't happened yet (the worklist processing comes a few lines later), and so checking if symbols exist in DstM wouldn't work.

I suppose we could move the !IsPerformingImport inline asm handling down here though. I don't think there'd be any downside to doing that after worklist processing rather than before. Would that be better?

llvm/test/ThinLTO/X86/import-symver.ll

9

Done.

[tejohnson]

llvm/lib/Linker/IRMover.cpp

1473

Ah yeah I see the issue. I don't think there would be any issue with moving that earlier handling down here though, so why don't you do that so all the inline asm handling is together.

[hans]

Moving the "Append the module inline asm string" code down, and simplifying it in the process.

[hans]

llvm/lib/Linker/IRMover.cpp

1473

Done.

llvm/test/Linker/link-arm-and-thumb-module-inline-asm.ll

16 ↗

(On Diff #308725)

The previous code would output an empty module asm "" line here. My simplification removes that, and also makes the test expectations a bit more explicit.

[tejohnson]

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

[hans]

In D92335#2426457, @tejohnson wrote:

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

Landed that separately as 45d8a7843253ec68367c26114a2aa6bff2a7a4bb

llvm/lib/Linker/CMakeLists.txt

 add_llvm_component_library(LLVMLinker
   IRMover.cpp
   LinkModules.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Linker
 
   DEPENDS
   intrinsics_gen
 
   LINK_COMPONENTS
   Core
+  Object
   Support
   TransformUtils
   )

llvm/lib/Linker/IRMover.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GVMaterializer.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/TypeFinder.h"
+#include "llvm/Object/ModuleSymbolTable.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include <utility>
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 // TypeMap implementation.
 //===----------------------------------------------------------------------===//
else
       NewGV =
           new GlobalVariable(DstM, TypeMap.get(SGV->getValueType()),
                              /*isConstant*/ false, GlobalValue::ExternalLinkage,
                              /*init*/ nullptr, SGV->getName(),
                              /*insertbefore*/ nullptr,
                              SGV->getThreadLocalMode(), SGV->getAddressSpace());
   }
 
   if (ForDefinition)
     NewGV->setLinkage(SGV->getLinkage());
   else if (SGV->hasExternalWeakLinkage())
     NewGV->setLinkage(GlobalValue::ExternalWeakLinkage);
 
   if (auto *NewGO = dyn_cast<GlobalObject>(NewGV)) {
     // Metadata for global variables and function declarations is copied eagerly.
     if (isa<GlobalVariable>(SGV) || SGV->isDeclaration())
       NewGO->copyMetadata(cast<GlobalObject>(SGV), 0);
if (!SrcM->getTargetTriple().empty()&&
     emitWarning("Linking two modules of different target triples: '" +
                 SrcM->getModuleIdentifier() + "' is '" +
                 SrcM->getTargetTriple() + "' whereas '" +
                 DstM.getModuleIdentifier() + "' is '" + DstM.getTargetTriple() +
                 "'\n");
 
   DstM.setTargetTriple(SrcTriple.merge(DstTriple));
 
-  // Append the module inline asm string.
-  if (!IsPerformingImport && !SrcM->getModuleInlineAsm().empty()) {
-    DstM.appendModuleInlineAsm(adjustInlineAsm(SrcM->getModuleInlineAsm(),
-                                               SrcTriple));
-  }
-
   // Loop over all of the linked values to compute type mappings.
   computeTypeMapping();
 
   std::reverse(Worklist.begin(), Worklist.end());
   while (!Worklist.empty()) {
     GlobalValue *GV = Worklist.back();
     Worklist.pop_back();
 
Error IRLinker::run() {
   DoneLinkingBodies = true;
   Mapper.addFlags(RF_NullMapMissingGlobalValues);
 
   // Remap all of the named MDNodes in Src into the DstM module. We do this
   // after linking GlobalValues so that MDNodes that reference GlobalValues
   // are properly remapped.
   linkNamedMDNodes();
 
+  if (!IsPerformingImport && !SrcM->getModuleInlineAsm().empty()) {
+    // Append the module inline asm string.
+    DstM.appendModuleInlineAsm(adjustInlineAsm(SrcM->getModuleInlineAsm(),
+                                               SrcTriple));
+  } else if (IsPerformingImport) {
+    // Import any symver directives for symbols in DstM.
+    ModuleSymbolTable::CollectAsmSymvers(*SrcM,
+                                         [&](StringRef Name, StringRef Alias) {
+      if (DstM.getNamedValue(Name)) {
+        SmallString<256> S(".symver ");
+        S += Name;
+        S += ", ";
+        S += Alias;
+        DstM.appendModuleInlineAsm(S);
+      }
+    });
+  }
+
   // Merge the module flags into the DstM module.
   return linkModuleFlagsMetadata();
 }
 
 IRMover::StructTypeKeyInfo::KeyTy::KeyTy(ArrayRef<Type *> E, bool P)
     : ETypes(E), IsPacked(P) {}
 
 IRMover::StructTypeKeyInfo::KeyTy::KeyTy(const StructType *ST)

llvm/test/ThinLTO/X86/Inputs/import-symver-foo.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+module asm ".symver bar, bar@BAR_1.2.3"
+
+declare dso_local i32 @bar()
+
+define dso_local i32 @foo() {
+entry:
+  %call = tail call i32 @bar()
+  ret i32 %call
+}

llvm/test/ThinLTO/X86/import-symver.ll

+; RUN: opt -thinlto-bc %s -o %t1.bc
+; RUN: opt -thinlto-bc %p/Inputs/import-symver-foo.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink %t1.bc %t2.bc -o %t3.index.bc
+
+; RUN: llvm-lto -thinlto-action=import -exported-symbol=main %t1.bc -thinlto-index=%t3.index.bc
+; RUN: llvm-dis %t1.bc.thinlto.imported.bc -o - | FileCheck --check-prefix=IMPORT %s
+
+; RUN: llvm-lto -thinlto-action=import -exported-symbol=main -import-instr-limit=0 %t1.bc -thinlto-index=%t3.index.bc
+; RUN: llvm-dis %t1.bc.thinlto.imported.bc -o - | FileCheck --check-prefix=NOIMPORT %s
+
+; When @bar gets imported, the symver must be imported too.
+; IMPORT: module asm ".symver bar, bar@BAR_1.2.3"
+; IMPORT: declare dso_local i32 @bar()
+
+; When @bar isn't imported, the symver is also not imported.
+; NOIMPORT-NOT: module asm
+; NOIMPORT-NOT: declare dso_local i32 @bar()
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare dso_local i32 @foo()
+
+define dso_local i32 @main() {
+entry:
+  %call = tail call i32 @foo()
+  ret i32 %call
+}

llvm/utils/gn/secondary/llvm/lib/Linker/BUILD.gn

 static_library("Linker") {
   output_name = "LLVMLinker"
   deps = [
     "//llvm/lib/IR",
+    "//llvm/lib/Object",
     "//llvm/lib/Support",
     "//llvm/lib/Transforms/Utils",
   ]
   sources = [
     "IRMover.cpp",
     "LinkModules.cpp",
   ]
 }