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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D92335#2425679 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Linker/IRMover.cpp:1484
+  // Import symver directives.
+  if (IsPerformingImport) {
+    ModuleSymbolTable::CollectAsmSymvers(*SrcM,
----------------
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.


================
Comment at: llvm/test/ThinLTO/X86/import-symver.ll:8
+
+; When @bar gets imported, the symver must be imported too.
+; CHECK: module asm ".symver bar, bar at BAR_1.2.3"
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92335/new/

https://reviews.llvm.org/D92335