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

[Kristina Bessonova via Phabricator via llvm-commits]

krisb created this revision.
krisb added reviewers: tejohnson, evgeny777.
Herald added subscribers: steven_wu, hiraditya, inglorion.
krisb requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Currently, if there is a module that contains a strong definition of
a global variable and a module that has both a weak definition for
the same global and a reference to it, it may result in an undefined symbol error
while linking with ThinLTO.

It happens because:

- the strong definition got internalized because it is read-only and can be imported;
- the weak definition got replaced by a declaration because it's non-prevailing;
- the strong definition failed to be imported because the destination module already contains another definition of the global yet this def is non-prevailing.

The patch adds a check to computeImportForReferencedGlobals() that allows
considering a global variable for being imported even if the module contains
a definition of it in the case this def has an interposable linkage type.

Note that currently the check is based only on the linkage type
(and this seems to be enough), but it might be worth to account the information
whether the def is prevailing or not.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95943

Files:
  llvm/lib/Transforms/IPO/FunctionImport.cpp
  llvm/test/ThinLTO/X86/Inputs/weak_globals_import.ll
  llvm/test/ThinLTO/X86/weak_globals_import.ll


Index: llvm/test/ThinLTO/X86/weak_globals_import.ll
===================================================================
--- /dev/null
+++ llvm/test/ThinLTO/X86/weak_globals_import.ll
@@ -0,0 +1,30 @@
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/weak_globals_import.ll -o %t2.bc
+
+; RUN: llvm-lto2 run -save-temps %t1.bc %t2.bc -o %t.out \
+; RUN:               -r=%t1.bc,main,plx \
+; RUN:               -r=%t1.bc,_Z3foov,l \
+; RUN:               -r=%t1.bc,G \
+; RUN:               -r=%t2.bc,_Z3foov,pl \
+; RUN:               -r=%t2.bc,G,pl
+; RUN: llvm-dis %t.out.1.3.import.bc -o -  | FileCheck %s
+; RUN: llvm-dis %t.out.2.3.import.bc -o -  | FileCheck %s
+
+; Test that a prevailing def for a global variable got imported even if
+; a destination module already contains non-prevailing one.
+
+; CHECK: @G = internal local_unnamed_addr global i32 42
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at G = weak dso_local local_unnamed_addr global i32 0, align 4
+
+define dso_local i32 @main() local_unnamed_addr {
+  %1 = load i32, i32* @G, align 4
+  %2 = tail call i32 @_Z3foov()
+  %3 = add nsw i32 %2, %1
+  ret i32 %3
+}
+
+declare dso_local i32 @_Z3foov() local_unnamed_addr
Index: llvm/test/ThinLTO/X86/Inputs/weak_globals_import.ll
===================================================================
--- /dev/null
+++ llvm/test/ThinLTO/X86/Inputs/weak_globals_import.ll
@@ -0,0 +1,11 @@
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at G = dso_local local_unnamed_addr global i32 42, align 4
+
+; Function Attrs: noinline
+define dso_local i32 @_Z3foov() local_unnamed_addr #0 {
+  ret i32 0
+}
+
+attributes #0 = { noinline }
Index: llvm/lib/Transforms/IPO/FunctionImport.cpp
===================================================================
--- llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -282,10 +282,18 @@
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
   for (auto &VI : Summary.refs()) {
-    if (DefinedGVSummaries.count(VI.getGUID())) {
-      LLVM_DEBUG(
-          dbgs() << "Ref ignored! Target already in destination module.\n");
-      continue;
+    // Find if the module already contains a suitable definition.
+    const auto &GVS = DefinedGVSummaries.find(VI.getGUID());
+    if (GVS != DefinedGVSummaries.end()) {
+      if (VI.getSummaryList().size() > 1 &&
+          GlobalValue::isInterposableLinkage(GVS->second->linkage())) {
+        // Give it a chance to be imported from somewhere else.
+        ;
+      } else {
+        LLVM_DEBUG(
+            dbgs() << "Ref ignored! Target already in destination module.\n");
+        continue;
+      }
     }
 
     LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D95943.321065.patch
Type: text/x-patch
Size: 2983 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210203/c016b287/attachment.bin>