[ThinLTO] Fix bug when importing writeonly variables

eleviant · eleviant · commit 7f92d66f3785 · 2019-11-08T20:50:34.000+03:00
Patch enables import of write-only variables with non-trivial initializers to fix linker errors. Initializers of imported variables are converted to 'zeroinitializer' to avoid promotion of referenced objects. Differential revision: https://reviews.llvm.org/D70006
diff --git a/llvm/lib/IR/ModuleSummaryIndex.cpp b/llvm/lib/IR/ModuleSummaryIndex.cpp
@@ -197,7 +197,21 @@ void ModuleSummaryIndex::propagateAttributes(
 bool ModuleSummaryIndex::canImportGlobalVar(GlobalValueSummary *S,
                                             bool AnalyzeRefs) const {
   auto HasRefsPreventingImport = [this](const GlobalVarSummary *GVS) {
-    return !isReadOnly(GVS) && GVS->refs().size();
+    // We don't analyze GV references during attribute propagation, so
+    // GV with non-trivial initializer can be marked either read or
+    // write-only.
+    // Importing definiton of readonly GV with non-trivial initializer
+    // allows us doing some extra optimizations (like converting indirect
+    // calls to direct).
+    // Definition of writeonly GV with non-trivial initializer should also
+    // be imported. Not doing so will result in:
+    // a) GV internalization in source module (because it's writeonly)
+    // b) Importing of GV declaration to destination module as a result
+    //    of promotion.
+    // c) Link error (external declaration with internal definition).
+    // However we do not promote objects referenced by writeonly GV
+    // initializer by means of converting it to 'zeroinitializer'
+    return !isReadOnly(GVS) && !isWriteOnly(GVS) && GVS->refs().size();
   };
   auto *GVS = cast<GlobalVarSummary>(S->getBaseObject());
 
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -318,10 +318,14 @@ static void computeImportForReferencedGlobals(
         if (ILI.second)
           NumImportedGlobalVarsThinLink++;
         MarkExported(VI, RefSummary.get());
-        // Promote referenced functions and variables
-        for (const auto &VI : RefSummary->refs())
-          for (const auto &RefFn : VI.getSummaryList())
-            MarkExported(VI, RefFn.get());
+        // Promote referenced functions and variables. We don't promote
+        // objects referenced by writeonly variable initializer, because
+        // we convert such variables initializers to "zeroinitializer".
+        // See processGlobalForThinLTO.
+        if (!Index.isWriteOnly(cast<GlobalVarSummary>(RefSummary.get())))
+          for (const auto &VI : RefSummary->refs())
+            for (const auto &RefFn : VI.getSummaryList())
+              MarkExported(VI, RefFn.get());
         break;
       }
   }
diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/InstIterator.h"
 using namespace llvm;
 
@@ -250,10 +251,20 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
       // We might have a non-null VI and get here even in that case if the name
       // matches one in this module (e.g. weak or appending linkage).
       auto* GVS = dyn_cast_or_null<GlobalVarSummary>(
-          ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
-      // At this stage "maybe" is "definitely"
-      if (GVS && (ImportIndex.isReadOnly(GVS) || ImportIndex.isWriteOnly(GVS)))
+          ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));      
+      if (GVS &&
+          (ImportIndex.isReadOnly(GVS) || ImportIndex.isWriteOnly(GVS))) {
         V->addAttribute("thinlto-internalize");
+        // Objects referenced by writeonly GV initializer should not be 
+        // promoted, because there is no any kind of read access to them
+        // on behalf of this writeonly GV. To avoid promotion we convert
+        // GV initializer to 'zeroinitializer'. This effectively drops
+        // references in IR module (not in combined index), so we can
+        // ignore them when computing import. We do not export references
+        // of writeonly object. See computeImportForReferencedGlobals
+        if (ImportIndex.isWriteOnly(GVS) && GVS->refs().size())
+          V->setInitializer(Constant::getNullValue(V->getValueType()));
+      }
     }
   }
 
diff --git a/llvm/test/ThinLTO/X86/Inputs/writeonly-with-refs.ll b/llvm/test/ThinLTO/X86/Inputs/writeonly-with-refs.ll
@@ -0,0 +1,17 @@
+; ModuleID = 'foo.o'
+source_filename = "foo.cpp"
+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"
+
+%struct.S = type { i32 }
+%struct.Q = type { %struct.S* }
+
+@_ZL3Obj = internal constant %struct.S { i32 42 }, align 4
+@outer = dso_local local_unnamed_addr global %struct.Q { %struct.S* @_ZL3Obj }, align 8
+
+; Function Attrs: nofree norecurse nounwind uwtable writeonly
+define dso_local void @_Z3foov() local_unnamed_addr {
+entry:
+  store %struct.S* null, %struct.S** getelementptr inbounds (%struct.Q, %struct.Q* @outer, i64 0, i32 0), align 8
+  ret void
+}
diff --git a/llvm/test/ThinLTO/X86/writeonly-with-refs.ll b/llvm/test/ThinLTO/X86/writeonly-with-refs.ll
@@ -0,0 +1,26 @@
+; RUN: opt -thinlto-bc %s -o %t1
+; RUN: opt -thinlto-bc %p/Inputs/writeonly-with-refs.ll -o %t2
+; RUN: llvm-lto2 run -save-temps %t1 %t2 -o %t-out \
+; RUN:    -r=%t1,main,plx \
+; RUN:    -r=%t1,_Z3foov,l \
+; RUN:    -r=%t2,_Z3foov,pl \
+; RUN:    -r=%t2,outer,pl
+
+; @outer should have been internalized and converted to zeroinitilizer.
+; RUN: llvm-dis %t-out.1.5.precodegen.bc -o - | FileCheck %s
+; RUN: llvm-dis %t-out.2.5.precodegen.bc -o - | FileCheck %s
+
+; CHECK: @outer = internal unnamed_addr global %struct.Q zeroinitializer
+
+source_filename = "main.cpp"
+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"
+
+; Function Attrs: norecurse uwtable
+define dso_local i32 @main() local_unnamed_addr {
+entry:
+  tail call void @_Z3foov()
+  ret i32 0
+}
+
+declare dso_local void @_Z3foov() local_unnamed_addr
