[PATCH] D95943: [ThinLTO] Fix import of a global variable to modules contain its weak def

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm with some comment tweaks suggested below for clarity. Thanks!



================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:292
+        // of a multiply defined global.
+        // First of all, this is required for the correctness purpose:
+        // if this copy is non-prevailing it's, basically, not a definition
----------------
Since the next two points (about the non-prevailing interposable becoming a decl, and then the read-only prevailing being internalized) are both about the correctness and need to go together to create the failure, can you reorder this to:
"This is required for the correctness purpose: First of all, if this copy is non-prevailing...."
Then reorder the second point slightly:
"On the other side, if the prevailing copy is read-only and suitable for import, it will be later internalized and thus not available for linking, and therefore must be imported to satisfy any references."


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:401
+      // FIXME: Consider not skipping import if the module contains a
+      // non-prevailing def with interposable linkage.
       LLVM_DEBUG(dbgs() << "ignored! Target already in destination module.\n");
----------------
Nit: For clarity, add: "The prevailing copy can safely be imported."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95943