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

Tue Dec 1 07:36:39 PST 2020

hans marked 2 inline comments as done.
hans added a comment.

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

================
Comment at: llvm/lib/Linker/IRMover.cpp:711

+  if (IsPerformingImport) {
+    // If there's a symver for the imported symbol, import it too.
----------------
tejohnson wrote:
> This function gets called once per global value being linked in, so it would redo the below code multiple times.
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.

================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:490
+  if (!Symvers.empty()) {
+    llvm::errs () << "writing symvers metadata\n";
+    NamedMDNode *NMD = M.getOrInsertNamedMetadata("symvers");
----------------
tejohnson wrote:
> leftover debugging output?
Oops, yes. But these changes are no longer necessary anyway.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92335