[PATCH] D95943: [ThinLTO] Fix import of a global variable to modules contain its weak def
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 15 09:01:08 PST 2021
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
More information about the llvm-commits
mailing list