[PATCH] D95943: [ThinLTO] Fix import of multiply defined global variables

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:288
+    if (GVS != DefinedGVSummaries.end()) {
+      if (VI.getSummaryList().size() > 1 &&
+          GlobalValue::isInterposableLinkage(GVS->second->linkage())) {
----------------
MaskRay wrote:
> Nit: flip the condition so that the "then" branch can be deleted, and the comment can be dedented by one level.
Extracted the check into a separate function. Hope it can be reused for computeImportForFunction() as well.


================
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
----------------
tejohnson wrote:
> 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."
Thank you for the suggestion! I rephrased the comment and hope it's more clear now.


================
Comment at: llvm/test/ThinLTO/X86/weak_globals_import.ll:1
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/weak_globals_import.ll -o %t2.bc
----------------
MaskRay wrote:
> Option: you may consider the `split-file` utility to keep the auxiliary file in this file. It may help readers in some cases as they don't need to open another file.
Thank you! I haven't heard about `split-file`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95943