[PATCH] D149630: [ThinLTO] Loosen up variable importing correctness checks

[Teresa Johnson via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG48f18ecd8293: [ThinLTO] Loosen up variable importing correctness checks (authored by tejohnson).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149630

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


Index: llvm/test/ThinLTO/X86/check_var_import_odr.ll
===================================================================
--- /dev/null
+++ llvm/test/ThinLTO/X86/check_var_import_odr.ll
@@ -0,0 +1,41 @@
+;; Test to ensure that we don't assert when checking variable importing
+;; correctness for read only variables where the importing module contains
+;; a linkonce_odr copy of the variable. In that case we do not need to
+;; import the read only variable even when it has been exported from its
+;; original module (in this case when we decided to import @bar).
+
+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/foo.ll -o %t/foo.o
+; RUN: opt -module-summary %t/bar.ll -o %t/bar.o
+; RUN: llvm-lto2 run %t/foo.o %t/bar.o -r=%t/foo.o,foo,pl -r=%t/foo.o,bar,l -r=%t/foo.o,qux,pl -r=%t/bar.o,bar,pl -r=%t/bar.o,qux, -o %t.out -save-temps
+; RUN: llvm-dis %t.out.1.3.import.bc -o - | FileCheck %s
+
+;; Check that we have internalized @qux (since it is read only), and imported @bar.
+; CHECK: @qux = internal global i32 0
+; CHECK: define available_externally hidden void @bar()
+
+;--- foo.ll
+
+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 qux = linkonce_odr hidden global i32 0
+
+define linkonce_odr hidden void @foo() {
+  call void @bar()
+  ret void
+}
+
+declare hidden void @bar()
+
+;--- bar.ll
+
+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 qux = linkonce_odr hidden global i32 0
+
+define hidden void @bar() {
+  %1 = load i32, i32* @qux, align 8
+  ret void
+}
Index: llvm/lib/Transforms/IPO/FunctionImport.cpp
===================================================================
--- llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -678,17 +678,23 @@
   // Checks that all GUIDs of read/writeonly vars we see in export lists
   // are also in the import lists. Otherwise we my face linker undefs,
   // because readonly and writeonly vars are internalized in their
-  // source modules.
-  auto IsReadOrWriteOnlyVar = [&](StringRef ModulePath, const ValueInfo &VI) {
+  // source modules. The exception would be if it has a linkage type indicating
+  // that there may have been a copy existing in the importing module (e.g.
+  // linkonce_odr). In that case we cannot accurately do this checking.
+  auto IsReadOrWriteOnlyVarNeedingImporting = [&](StringRef ModulePath,
+                                                  const ValueInfo &VI) {
     auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
         Index.findSummaryInModule(VI, ModulePath));
-    return GVS && (Index.isReadOnly(GVS) || Index.isWriteOnly(GVS));
+    return GVS && (Index.isReadOnly(GVS) || Index.isWriteOnly(GVS)) &&
+           !(GVS->linkage() == GlobalValue::AvailableExternallyLinkage ||
+             GVS->linkage() == GlobalValue::WeakODRLinkage ||
+             GVS->linkage() == GlobalValue::LinkOnceODRLinkage);
   };
 
   for (auto &ExportPerModule : ExportLists)
     for (auto &VI : ExportPerModule.second)
       if (!FlattenedImports.count(VI.getGUID()) &&
-          IsReadOrWriteOnlyVar(ExportPerModule.first(), VI))
+          IsReadOrWriteOnlyVarNeedingImporting(ExportPerModule.first(), VI))
         return false;
 
   return true;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D149630.518732.patch
Type: text/x-patch
Size: 3385 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230502/6d2b02d9/attachment.bin>