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

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


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


================
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"
----------------
tejohnson wrote:
> 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.
Done.


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

https://reviews.llvm.org/D92335