[ThinLTO] Fix bug when importing writeonly variables

[evgeny777]

Summary

D69561introduced a bug which causes writeonly variable with non-trivial initializer (with references) to be internalized in source module and its declaration imported to destination module. This causes linker errors. To fix this bug I suggest importing such variable definitions, but convert them to zeroinitializer before doing so in order to avoid promotion of referenced objects which are not accessed anyway.

[tejohnson]

We hit a linker undef also because of D69561. I just confirmed this patch fixes the issue. It would be good to get this patch in asap or revert the original change. I have some questions on this patch though.

I'm a little confused about what causes the original failure mode, and whether this patch is an enhancement that essentially hides the underlying bug or is really fixing the underlying issue. Since D69561 only affected read only variables, how did it affect a write only variable?

llvm/lib/Transforms/IPO/FunctionImport.cpp

321–328

Needs comment

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

258

Needs comment

260

What if it isn't a struct or array? Is that the only situation where we can hit this problem?

[evgeny777]

I'm a little confused about what causes the original failure mode

Assuming we have writeonly variable foo with non-trivial initializer, the following happens after D69561

1. foo is marked as writeonly due to change in canImportGlobalVar
2. computeImportForReferencedGlobals refuses to import foo because !isReadOnly(GVS) && GVS->refs().size() evaluates to true. See HasReferencesPreventingImport
3. Import of function which uses foo causes declaration of foo to be exported to destination module
4. foo is internalized in source module because due to being "writeonly"

[evgeny777]

This comment has been deleted.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

260

Looks like you're right. I'll replace with Constant::getNullValue and update the patch

[evgeny777]

Addressed

[tejohnson]

Ah, ok, I wasn't thinking about the part where the flag propagation was changed. I'm a little concerned that due to that change we are now required to import the variable as a definition. I guess there is no current way to disable importing of references while leaving on importing of functions (e.g. through some kind of internal option for debugging), but since such a mechanism might be useful at some point for debugging, my concern is that it will cause failures. Please add some very clear comments to the code where you are checking the write only flag as to why and what is going on. I.e. why needed for correctness and how we can ignore its references (because it will eventually be made zeroinit). Also, it would be good to add an assert somewhere to ensure that we don't export a reference but not a definition for a write only variable, to guard against any regression if someone does add a debugging mechanism to disable variable importing (which would need to affect the flag propagation). Maybe down at the bottom of ComputeCrossModuleImport where we prune the ExportLists.

[tejohnson]

A few suggestions for beefing up the comments

llvm/lib/IR/ModuleSummaryIndex.cpp

200–214

Please add comment about why refs on write only vars must be ignored for correctness, and that we must import their defs.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

261

Please add a note that we have to do this as the variable importing code does not mark these references as exported.

[steven_wu]

I think this is probably the best fix I can think as well. Agree with Teresa says, please add comments for reasons why it is implemented this way.

[evgeny777]

Addressed

[tejohnson]

Let's get this one in (I have a couple more comment nits below), to fix the immediate problem. But please send a follow on patch soon to do the assertion checking I suggested in my earlier comment, to make sure we catch any regressions to the force import of these write only variables.

llvm/lib/IR/ModuleSummaryIndex.cpp

204

Add that this is because we don't AnalyzeRefs when propagating the write only property.

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

260

Please add that this is because we do not export their references.

[evgeny777]

But please send a follow on patch soon to do the assertion checking I suggested in my earlier comment

Just pushed this in. It's getting late here, so feel free to revert if things go bad. If not, I'll send follow-up with assertion on Monday.

llvm/lib/IR/ModuleSummaryIndex.cpp

for (auto &P : *this)
             if (GVS->maybeWriteOnly())
               WriteOnlyLiveGVars++;
           }
 }
 
 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());
 
   // Global variable with non-trivial initializer can be imported
   // if it's readonly. This gives us extra opportunities for constant
   // folding and converting indirect calls to direct calls. We don't
   // analyze GV references during attribute propagation, because we
   // don't know yet if it is readonly or not.

llvm/lib/Transforms/IPO/FunctionImport.cpp

for (auto &RefSummary : VI.getSummaryList())
       if (isa<GlobalVarSummary>(RefSummary.get()) &&
           Index.canImportGlobalVar(RefSummary.get(), /* AnalyzeRefs */ true) &&
           !LocalNotInModule(RefSummary.get())) {
         auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
         // Only update stat if we haven't already imported this variable.
         if (ILI.second)
           NumImportedGlobalVarsThinLink++;
         MarkExported(VI, RefSummary.get());
-        // Promote referenced functions and variables
+        // 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;
       }
   }
 }
 
 static const char *
 getFailureName(FunctionImporter::ImportFailureReason Reason) {
   switch (Reason) {

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

 //===- lib/Transforms/Utils/FunctionImportUtils.cpp - Importing utilities -===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the FunctionImportGlobalProcessing class, used
 // to perform the necessary global value handling for function importing.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/InstIterator.h"
 using namespace llvm;
 
 /// Checks if we should import SGV as a definition, otherwise import as a
 /// declaration.
 bool FunctionImportGlobalProcessing::doImportAsDefinition(
     const GlobalValue *SGV, SetVector<GlobalValue *> *GlobalsToImport) {
 
if (GlobalVariable *V = dyn_cast<GlobalVariable>(&GV)) {
       // compiled in their respective directories (so the source file name
       // and resulting GUID is the same). Find the one in this module.
       // Handle the case where there is no summary found in this module. That
       // can happen in the distributed ThinLTO backend, because the index only
       // contains summaries from the source modules if they are being imported.
       // 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()));
+      }
     }
   }
 
   bool DoPromote = false;
   if (GV.hasLocalLinkage() &&
       ((DoPromote = shouldPromoteLocalToGlobal(&GV)) || isPerformingImport())) {
     // Save the original name string before we rename GV below.
     auto Name = GV.getName().str();

llvm/test/ThinLTO/X86/Inputs/writeonly-with-refs.ll

+; 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
+}

llvm/test/ThinLTO/X86/writeonly-with-refs.ll

+; 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