Always traverse GlobalVariable initializer when computing the export list

[mehdi_amini]

Summary

We are always importing the initializer for a GlobalVariable.
So if a GlobalVariable is in the export-list, we pull in any
refs as well.

[tejohnson]

We are always importing the initializer for a GlobalVariable.

I don't think this is true anymore, or at least I can't find where we are doing this. We used to always import variable defs in the Linker, but now that is being passed a list of globals to import.

Now we are skipping variables with a comment:

auto *Summary = dyn_cast_or_null<FunctionSummary>(GlobInfo->summary());
if (!Summary)
  /// Ignore global variable, focus on functions
  continue;

and computeImportForFunction does not walk refs() looking for possible imports. Am I missing something?

I think if we want to add handling like this for the future when variable defs are imported, a better place would be in computeImportForFunction where we would presumably walk the refs() and make those decisions (similar to how the ExportList is updated for callees in the loop over the call edges in that function.

[mehdi_amini]

In D19102#401545, @tejohnson wrote:

We are always importing the initializer for a GlobalVariable.

I don't think this is true anymore, or at least I can't find where we are doing this. We used to always import variable defs in the Linker, but now that is being passed a list of globals to import.

We do it for every global that is referenced from the IR we import, this is done in IRLinker::linkGlobalInit(GlobalVariable &Dst, GlobalVariable &Src).
(I couldn't find a reliable way to avoid this, too many corner cases).

I think if we want to add handling like this for the future when variable defs are imported, a better place would be in computeImportForFunction where we would presumably walk the refs() and make those decisions (similar to how the ExportList is updated for callees in the loop over the call edges in that function.

That's was the plan when we moved to the "reference graph", however it seems to me that we're blocked on IRLinker::linkGlobalInit(GlobalVariable &Dst, GlobalVariable &Src) right now.

I'll update the comment mentionning that it is a current limitation of the IRMover.

[mehdi_amini]

Update comment to reflect that it is a workaround for an IRMover limitation

[tejohnson]

In D19102#401562, @joker.eph wrote:

In D19102#401545, @tejohnson wrote:

We are always importing the initializer for a GlobalVariable.

I don't think this is true anymore, or at least I can't find where we are doing this. We used to always import variable defs in the Linker, but now that is being passed a list of globals to import.

We do it for every global that is referenced from the IR we import, this is done in IRLinker::linkGlobalInit(GlobalVariable &Dst, GlobalVariable &Src).
(I couldn't find a reliable way to avoid this, too many corner cases).

I don't believe that is true that we always link in inits for global variables that are referenced. Yes linkGlobalInit is called for variables unconditionally from linkGlobalValueBody, but then, so is linkFunctionBody for functions from the same place. We invoke linkGlobalValueBody from materializeInitFor, which I believe is unconditionally invoked on references from the ValueMapper. However, materializeInitFor only invokes linkGlobalValueBody only when "ForAlias || shouldLink(New, *Old)".

Looking at when those conditions are true it seems like there are therefore a few conditions under which we would map in a global variable init (although I don't see anything specific to variables here):

1. ForAlias==true: This happens when this value was encountered when mapping an aliasee, which could be a variable
2. New is in ValuesToLink: Not currently true for global variables
3. New hasLocalLinkage(): We should have promoted any locals referenced by an import before this, so shouldn't be true here
4. AddLazyFor returns true: Aha - you are probably catching some variables here due to the issue with addLazyFor currently force importing linkonce!

So I think the issue is not with linkGlobalInit, but rather the current Linker behavior around aliases and linkonce (and I don't think this is specific to variables). Again, correct me if I am missing something though.

[mehdi_amini]

In D19102#401669, @tejohnson wrote:

In D19102#401562, @joker.eph wrote:

In D19102#401545, @tejohnson wrote:

We are always importing the initializer for a GlobalVariable.

I don't think this is true anymore, or at least I can't find where we are doing this. We used to always import variable defs in the Linker, but now that is being passed a list of globals to import.

We do it for every global that is referenced from the IR we import, this is done in IRLinker::linkGlobalInit(GlobalVariable &Dst, GlobalVariable &Src).
(I couldn't find a reliable way to avoid this, too many corner cases).

I don't believe that is true that we always link in inits for global variables that are referenced. Yes linkGlobalInit is called for variables unconditionally from linkGlobalValueBody, but then, so is linkFunctionBody for functions from the same place. We invoke linkGlobalValueBody from materializeInitFor, which I believe is unconditionally invoked on references from the ValueMapper. However, materializeInitFor only invokes linkGlobalValueBody only when "ForAlias || shouldLink(New, *Old)".

Looking at when those conditions are true it seems like there are therefore a few conditions under which we would map in a global variable init (although I don't see anything specific to variables here):

1. ForAlias==true: This happens when this value was encountered when mapping an aliasee, which could be a variable
2. New is in ValuesToLink: Not currently true for global variables
3. New hasLocalLinkage(): We should have promoted any locals referenced by an import before this, so shouldn't be true here
4. AddLazyFor returns true: Aha - you are probably catching some variables here due to the issue with addLazyFor currently force importing linkonce!

So I think the issue is not with linkGlobalInit, but rather the current Linker behavior around aliases and linkonce (and I don't think this is specific to variables). Again, correct me if I am missing something though.

Right, so I didn't point to the right API, I have *another* patch (D19096) for the issue with linkGlobalInit (that comes from addLazyFor indeed).

For this patch it was maybe rather for *constant* that we will always import? (but we don't know in the summary if a GlobalVariable is constant right?)

[mehdi_amini]

I think this is obsolete now, it was indeed global linkonce odr that were pulled in (like Vtable for instance) as well as the linkonce_odr member function referenced by it.

[mehdi_amini]

D19096 fixed it

[mehdi_amini]

I can't bootstrap clang locally without this patch, so I need to extract a reduced test case to motivate it

[mehdi_amini]

Add a test that illustrates the bug fixed here.

[tejohnson]

Just needs comment update now that we finally know why gvar init is being imported in this case.

lib/Transforms/IPO/FunctionImport.cpp

173

Can you update the comment which is inaccurate since we don't always import initializers? Oh, I just figured out why this is getting imported. It is related to #3 from my earlier list of when materializeInitFor will decide to linkGlobalValueBody():

3. New hasLocalLinkage(): We should have promoted any locals referenced by an import before this, so shouldn't be true here

But I was wrong! The one situation where we will not promote a local is a global variable that is constant with unnamed addr:

bool FunctionImportGlobalProcessing::doPromoteLocalToGlobal(
...

auto *GVar = dyn_cast<GlobalVariable>(SGV);
if (GVar && GVar->isConstant() && GVar->hasUnnamedAddr())
  return false;

which is exactly what happens in your test case. Can you update the comment accordingly?

Also, maybe add a FIXME that if we had a flag in summary indicating that GVar was constant that we could refine this?

[mehdi_amini]

lib/Transforms/IPO/FunctionImport.cpp

173

I'll update.
(added a note in our TODO shared doc as well)

[mehdi_amini]

Update before commit

[mehdi_amini]

r267303

lib/Transforms/IPO/FunctionImport.cpp

static const GlobalValueSummary *selectCallee(GlobalValue::GUID GUID,
                                               const ModuleSummaryIndex &Index) {
   auto CalleeInfoList = Index.findGlobalValueInfoList(GUID);
   if (CalleeInfoList == Index.end()) {
     return nullptr; // This function does not have a summary
   }
   return selectCallee(CalleeInfoList->second, Threshold);
 }
 
-/// Return true if the global \p GUID is exported by module \p ExportModulePath.
-static bool isGlobalExported(const ModuleSummaryIndex &Index,
+/// Mark the global \p GUID as export by module \p ExportModulePath if found in
+/// this module. If it is a GlobalVariable, we also mark any referenced global
+/// in the current module as exported.
+static void exportGlobalInModule(const ModuleSummaryIndex &Index,
                                  StringRef ExportModulePath,
-                             GlobalValue::GUID GUID) {
-  auto CalleeInfoList = Index.findGlobalValueInfoList(GUID);
-  if (CalleeInfoList == Index.end())
+                                 GlobalValue::GUID GUID,
+                                 FunctionImporter::ExportSetTy &ExportList) {
+  auto FindGlobalInfoInModule =
+      [&](GlobalValue::GUID GUID) -> GlobalValueInfo *{
+        auto InfoList = Index.findGlobalValueInfoList(GUID);
+        if (InfoList == Index.end())
           // This global does not have a summary, it is not part of the ThinLTO
           // process
-    return false;
-  auto DefinedInCalleeModule = llvm::find_if(
-      CalleeInfoList->second,
+          return nullptr;
+        auto Info = llvm::find_if(
+            InfoList->second,
             [&](const std::unique_ptr<GlobalValueInfo> &GlobInfo) {
               auto *Summary = GlobInfo->summary();
               assert(Summary && "Unexpected GlobalValueInfo without summary");
               return Summary->modulePath() == ExportModulePath;
             });
-  return (DefinedInCalleeModule != CalleeInfoList->second.end());
+        if (Info == InfoList->second.end())
+          return nullptr;
+        return Info->get();
+      };
+
+  auto *GVInfo = FindGlobalInfoInModule(GUID);
+  if (!GVInfo)
+    return;
+  // We found it in the current module, mark as exported
+  ExportList.insert(GUID);
+
+  auto *Summary = GVInfo->summary();
+  auto GVS = dyn_cast<GlobalVarSummary>(Summary);
+  if (!GVS)
+    return;
+  // FunctionImportGlobalProcessing::doPromoteLocalToGlobal() will always
+  // trigger importing  the initializer for `constant unnamed addr` globals that
+  // are referenced. We conservatively export all the referenced symbols for
+  // every global to workaround this, so that the ExportList is accurate.
+  // FIXME: with a "isConstant" flag in the summary we could be more targetted.
+  for (auto &Ref : GVS->refs()) {
+    auto GUID = Ref.getGUID();
+    auto *RefInfo = FindGlobalInfoInModule(GUID);
+    if (RefInfo)
+      // Found a ref in the current module, mark it as exported
+      ExportList.insert(GUID);
+  }
 }
 
 using EdgeInfo = std::pair<const FunctionSummary *, unsigned /* Threshold */>;
 
 /// Compute the list of functions to import for a given caller. Mark these
 /// imported functions and the symbols they reference in their source module as
 /// exported from their source module.
 static void computeImportForFunction(
for (auto &Edge : Summary.calls()) {
     // Make exports in the source module.
     if (ExportLists) {
       auto &ExportList = (*ExportLists)[ExportModulePath];
       ExportList.insert(GUID);
       // Mark all functions and globals referenced by this function as exported
       // to the outside if they are defined in the same source module.
       for (auto &Edge : ResolvedCalleeSummary->calls()) {
         auto CalleeGUID = Edge.first.getGUID();
-        if (isGlobalExported(Index, ExportModulePath, CalleeGUID))
-          ExportList.insert(CalleeGUID);
+        exportGlobalInModule(Index, ExportModulePath, CalleeGUID, ExportList);
       }
       for (auto &Ref : ResolvedCalleeSummary->refs()) {
         auto GUID = Ref.getGUID();
-        if (isGlobalExported(Index, ExportModulePath, GUID))
-          ExportList.insert(GUID);
+        exportGlobalInModule(Index, ExportModulePath, GUID, ExportList);
       }
     }
 
     // Insert the newly imported function to the worklist.
     Worklist.push_back(std::make_pair(ResolvedCalleeSummary, Threshold));
   }
 }
 

test/ThinLTO/X86/Inputs/referenced_by_constant.ll

+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+define void @referencedbyglobal() {
+    ret void
+}
+
+@someglobal = internal unnamed_addr constant i8* bitcast (void ()* @referencedbyglobal to i8*)
+@ptr = global i8** null
+
+define  void @bar() #0 align 2 {
+  store i8** getelementptr inbounds (i8*, i8** @someglobal, i64 0) , i8*** @ptr, align 8
+  ret void
+}
+

test/ThinLTO/X86/referenced_by_constant.ll

+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary %s -o %t.bc
+; RUN: opt -module-summary %p/Inputs/referenced_by_constant.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
+
+; Check the import side: we import bar() and @someglobal, but not @referencedbyglobal()
+; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t3.bc -o - | llvm-dis -o -   | FileCheck %s --check-prefix=IMPORT
+; IMPORT: @someglobal.llvm.0 =
+; IMPORT: define available_externally void @bar()
+; IMPORT: declare void @referencedbyglobal()
+
+; Ensure that @referencedbyglobal() is not internalized
+; RUN: llvm-lto -thinlto-action=internalize %t2.bc -thinlto-index=%t3.bc -o - -exported-symbol=foo | llvm-dis -o - | FileCheck %s --check-prefix=INTERNALIZE
+; INTERNALIZE: define void @referencedbyglobal
+
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+declare void @bar()
+
+define void @foo() {
+    call void @bar()
+    ret void
+}
+