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

Tue Dec 1 11:47:57 PST 2020

hans marked an inline comment as done.
hans added inline comments.

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

================
Comment at: llvm/test/Linker/link-arm-and-thumb-module-inline-asm.ll:16
-; CHECK-NEXT: add r1, r2, r2
-; CHECK-NEXT: module asm
-; CHECK-NEXT: .text
----------------
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.

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

https://reviews.llvm.org/D92335