[ThinLTO]Supports declaration import for global variables in distributed ThinLTO

[mingmingl-llvm]

When -import-declaration option is enabled, declaration import is supported for functions. #88024 has the context for this option.

This patch supports declaration import for global variables in distributed ThinLTO. The motivating use case is to propagate dso_local attribute of global variables across modules, to optimize global variable access when a binary is built with -fno-direct-access-external-data.

• With -fdirect-access-external-data, non thread-local global variables will have dso_local attributes. This optimizes the global variable access as shown by https://gcc.godbolt.org/z/vMzWcKdh3

[llvmbot]

@llvm/pr-subscribers-lto
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Mingming Liu (mingmingl-llvm)

Changes

When -import-declaration option is enabled, declaration import is supported for functions. #88024 has the context for this option.

This patch supports declaration import for global variables in distributed ThinLTO. The motivating use case is to propagate dso_local attribute of global variables across modules, to optimize global variable access when a binary is built with -fno-direct-access-external-data.

• With -fdirect-access-external-data, non thread-local global variables will have dso_local attributes. This optimizes the global variable access as shown by https://gcc.godbolt.org/z/vMzWcKdh3

---

Full diff: https://github.com/llvm/llvm-project/pull/117616.diff

5 Files Affected:

• (modified) llvm/include/llvm/IR/ModuleSummaryIndex.h (+5)
• (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+2-1)
• (modified) llvm/lib/IR/ModuleSummaryIndex.cpp (+16-2)
• (modified) llvm/lib/Transforms/IPO/FunctionImport.cpp (+10-2)
• (modified) llvm/test/ThinLTO/X86/import_callee_declaration.ll (+42-6)

diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index 39c60229aa1d81..259391cbb718ba 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -1909,6 +1909,11 @@ class ModuleSummaryIndex {
 
   /// Checks if we can import global variable from another module.
   bool canImportGlobalVar(const GlobalValueSummary *S, bool AnalyzeRefs) const;
+
+  /// Same as above but checks whether the global var is importable as a
+  /// declaration.
+  bool canImportGlobalVar(const GlobalValueSummary *S, bool AnalyzeRefs,
+                          bool &CanImportDecl) const;
 };
 
 /// GraphTraits definition to build SCC for the index
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 24a4c2e8303d5a..dd6b97d521c4ae 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -4764,7 +4764,8 @@ void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
       NameVals.push_back(*ValueId);
       assert(ModuleIdMap.count(VS->modulePath()));
       NameVals.push_back(ModuleIdMap[VS->modulePath()]);
-      NameVals.push_back(getEncodedGVSummaryFlags(VS->flags()));
+      NameVals.push_back(
+          getEncodedGVSummaryFlags(VS->flags(), shouldImportValueAsDecl(VS)));
       NameVals.push_back(getEncodedGVarFlags(VS->varflags()));
       for (auto &RI : VS->refs()) {
         auto RefValueId = getValueId(RI.getGUID());
diff --git a/llvm/lib/IR/ModuleSummaryIndex.cpp b/llvm/lib/IR/ModuleSummaryIndex.cpp
index 12a558b3bc1b12..d9024b0a8673f1 100644
--- a/llvm/lib/IR/ModuleSummaryIndex.cpp
+++ b/llvm/lib/IR/ModuleSummaryIndex.cpp
@@ -328,6 +328,13 @@ void ModuleSummaryIndex::propagateAttributes(
 
 bool ModuleSummaryIndex::canImportGlobalVar(const GlobalValueSummary *S,
                                             bool AnalyzeRefs) const {
+  bool CanImportDecl;
+  return canImportGlobalVar(S, AnalyzeRefs, CanImportDecl);
+}
+
+bool ModuleSummaryIndex::canImportGlobalVar(const GlobalValueSummary *S,
+                                            bool AnalyzeRefs,
+                                            bool &CanImportDecl) const {
   auto HasRefsPreventingImport = [this](const GlobalVarSummary *GVS) {
     // We don't analyze GV references during attribute propagation, so
     // GV with non-trivial initializer can be marked either read or
@@ -348,13 +355,20 @@ bool ModuleSummaryIndex::canImportGlobalVar(const GlobalValueSummary *S,
   };
   auto *GVS = cast<GlobalVarSummary>(S->getBaseObject());
 
+  const bool nonInterposable =
+      !GlobalValue::isInterposableLinkage(S->linkage());
+  const bool eligibleToImport = !S->notEligibleToImport();
+
+  // It's correct to import a global variable only when it is not interposable
+  // and eligible to import.
+  CanImportDecl = (nonInterposable && eligibleToImport);
+
   // 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.
-  return !GlobalValue::isInterposableLinkage(S->linkage()) &&
-         !S->notEligibleToImport() &&
+  return nonInterposable && eligibleToImport &&
          (!AnalyzeRefs || !HasRefsPreventingImport(GVS));
 }
 
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index fee27f72f208b0..ca15a4b9e1be84 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -430,10 +430,18 @@ class GlobalsImporter final {
         // than as part of the logic deciding which functions to import (i.e.
         // based on profile information). Should we decide to handle them here,
         // we can refactor accordingly at that time.
-        if (!GVS || !Index.canImportGlobalVar(GVS, /* AnalyzeRefs */ true) ||
+        bool CanImportDecl = false;
+        if (!GVS ||
             shouldSkipLocalInAnotherModule(GVS, VI.getSummaryList().size(),
-                                           Summary.modulePath()))
+                                           Summary.modulePath()) ||
+            !Index.canImportGlobalVar(GVS, /* AnalyzeRefs */ true,
+                                      CanImportDecl)) {
+          if (ImportDeclaration && CanImportDecl)
+            ImportList.maybeAddDeclaration(RefSummary->modulePath(),
+                                           VI.getGUID());
+
           continue;
+        }
 
         // If there isn't an entry for GUID, insert <GUID, Definition> pair.
         // Otherwise, definition should take precedence over declaration.
diff --git a/llvm/test/ThinLTO/X86/import_callee_declaration.ll b/llvm/test/ThinLTO/X86/import_callee_declaration.ll
index 246920e5db0dc8..fd62aeb3f74401 100644
--- a/llvm/test/ThinLTO/X86/import_callee_declaration.ll
+++ b/llvm/test/ThinLTO/X86/import_callee_declaration.ll
@@ -34,11 +34,15 @@
 ; RUN:   -r=main.bc,main,px \
 ; RUN:   -r=main.bc,small_func, \
 ; RUN:   -r=main.bc,large_func, \
+; RUN:   -r=main.bc,read_write_global_vars, \
+; RUN:   -r=main.bc,external_func, \
 ; RUN:   -r=lib.bc,callee,pl \
 ; RUN:   -r=lib.bc,large_indirect_callee,px \
 ; RUN:   -r=lib.bc,large_indirect_bar,px \
 ; RUN:   -r=lib.bc,small_func,px \
 ; RUN:   -r=lib.bc,large_func,px \
+; RUN:   -r=lib.bc,read_write_global_vars,px \
+; RUN:   -r=lib.bc,external_func, \
 ; RUN:   -r=lib.bc,large_indirect_callee_alias,px \
 ; RUN:   -r=lib.bc,large_indirect_bar_alias,px \
 ; RUN:   -r=lib.bc,calleeAddrs,px -r=lib.bc,calleeAddrs2,px -o summary main.bc lib.bc 2>&1 | FileCheck %s --check-prefix=DUMP
@@ -46,7 +50,7 @@
 ; RUN: llvm-lto -thinlto-action=thinlink -import-declaration -import-instr-limit=7 -import-instr-evolution-factor=1.0 -o combined.index.bc main.bc lib.bc
 ; RUN: llvm-lto -thinlto-action=distributedindexes -debug-only=function-import -import-declaration -import-instr-limit=7 -import-instr-evolution-factor=1.0 -thinlto-index combined.index.bc main.bc lib.bc 2>&1 | FileCheck %s --check-prefix=DUMP
 
-; DUMP: - 2 function definitions and 4 function declarations imported from lib.bc
+; DUMP: - 2 function definitions and 3 function declarations imported from lib.bc
 
 ; First disassemble per-module summary and find out the GUID for {large_func, large_indirect_callee}.
 ;
@@ -67,17 +71,26 @@
 ; MAIN-DIS: gv: (guid: 2418497564662708935, summaries: (function: (module: [[LIBMOD]], flags: ({{.*}} importType: declaration), insts: 8, {{.*}})))
 ; When alias is imported as a copy of the aliasee, but the aliasee is not being
 ; imported by itself, the aliasee should be null.
-; MAIN-DIS: gv: (guid: 13590951773474913315, summaries: (alias: (module: [[LIBMOD]], flags: ({{.*}} importType: declaration), aliasee: null)))
+; MAIN-DIS-NOT: gv: (guid: 13590951773474913315, summaries: (alias: (module: [[LIBMOD]], flags: ({{.*}} importType: declaration), aliasee: null)))
 ; MAIN-DIS: [[LARGEINDIRECT:\^[0-9]+]] = gv: (guid: 14343440786664691134, summaries: (function: (module: [[LIBMOD]], flags: ({{.*}} importType: declaration), insts: 8, {{.*}})))
 ; MAIN-DIS: gv: (guid: 16730173943625350469, summaries: (alias: (module: [[LIBMOD]], flags: ({{.*}} importType: declaration), aliasee: [[LARGEINDIRECT]])))
 
+; RUN: opt -passes=function-import -summary-file=main.bc.thinlto.bc main.bc -o main-after-import.bc
+; RUN: llvm-dis -o - main-after-import.bc | FileCheck %s --check-prefix=MAIN-IMPORT
+
+; Tests that dso_local attribute is applied on a global var from its summary.
+MAIN-IMPORT: @read_write_global_vars = external dso_local global [1 x ptr]
+
 ; Run in-process ThinLTO and tests that
 ; 1. `callee` remains internalized even if the symbols of its callers
 ; (large_func, large_indirect_callee, large_indirect_bar) are exported as
 ; declarations and visible to main module.
 ; 2. the debugging logs from `function-import` pass are expected.
+; Set relocation model to static so the dso_local attribute from a summary is
+; applied on the global variable declaration.
 
 ; RUN: llvm-lto2 run \
+; RUN:   -relocation-model=static \
 ; RUN:   -debug-only=function-import \
 ; RUN:   -save-temps \
 ; RUN:   -thinlto-threads=1 \
@@ -87,11 +100,15 @@
 ; RUN:   -r=main.bc,main,px \
 ; RUN:   -r=main.bc,small_func, \
 ; RUN:   -r=main.bc,large_func, \
+; RUN:   -r=main.bc,read_write_global_vars, \
+; RUN:   -r=main.bc,external_func, \
 ; RUN:   -r=lib.bc,callee,pl \
 ; RUN:   -r=lib.bc,large_indirect_callee,px \
 ; RUN:   -r=lib.bc,large_indirect_bar,px \
 ; RUN:   -r=lib.bc,small_func,px \
 ; RUN:   -r=lib.bc,large_func,px \
+; RUN:   -r=lib.bc,read_write_global_vars,px \
+; RUN:   -r=lib.bc,external_func, \
 ; RUN:   -r=lib.bc,large_indirect_callee_alias,px \
 ; RUN:   -r=lib.bc,large_indirect_bar_alias,px \
 ; RUN:   -r=lib.bc,calleeAddrs,px -r=lib.bc,calleeAddrs2,px -o in-process main.bc lib.bc 2>&1 | FileCheck %s --check-prefix=IMPORTDUMP
@@ -103,9 +120,9 @@
 ; IMPORTDUMP-DAG: Is importing function definition 13568239288960714650 small_indirect_callee from lib.cc
 ; IMPORTDUMP-DAG: Is importing function definition 6976996067367342685 small_func from lib.cc
 ; IMPORTDUMP-DAG: Is importing function declaration 2418497564662708935 large_func from lib.cc
-; IMPORTDUMP-DAG: Not importing global 7680325410415171624 calleeAddrs from lib.cc
+; IMPORTDUMP-DAG: Is importing global declaration 7680325410415171624 calleeAddrs from lib.cc
 ; IMPORTDUMP-DAG: Is importing alias declaration 16730173943625350469 large_indirect_callee_alias from lib.cc
-; IMPORTDUMP-DAG: Is importing alias declaration 13590951773474913315 large_indirect_bar_alias from lib.cc
+; IMPORTDUMP-DAG: Not importing alias 13590951773474913315 large_indirect_bar_alias from lib.cc
 ; IMPORTDUMP-DAG: Not importing function 13770917885399536773 large_indirect_bar
 
 ; RUN: llvm-dis in-process.1.3.import.bc -o - | FileCheck %s --check-prefix=IMPORT
@@ -115,6 +132,8 @@
 ; IMPORT-DAG: define available_externally void @small_func
 ; IMPORT-DAG: define available_externally hidden void @small_indirect_callee
 ; IMPORT-DAG: declare void @large_func
+; Tests that dso_local attribute is applied on a global var from its summary.
+; IMPORT-DAG: @read_write_global_vars = external dso_local global [1 x ptr]
 ; IMPORT-NOT: large_indirect_callee
 ; IMPORT-NOT: large_indirect_callee_alias
 ; IMPORT-NOT: large_indirect_bar
@@ -126,9 +145,14 @@
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
+@read_write_global_vars = external global [1 x ptr]
+
 define i32 @main() {
   call void @small_func()
   call void @large_func()
+  %num = call ptr @external_func(ptr @read_write_global_vars)
+  store ptr %num, ptr getelementptr inbounds ([1 x ptr], ptr @read_write_global_vars, i64 0, i64 0)
+  %res1 = call i32 @external_func(ptr @read_write_global_vars)
   ret i32 0
 }
 
@@ -137,6 +161,8 @@ declare void @small_func()
 ; large_func without attributes
 declare void @large_func()
 
+declare ptr @external_func(ptr)
+
 ;--- lib.ll
 source_filename = "lib.cc"
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
@@ -149,6 +175,10 @@ target triple = "x86_64-unknown-linux-gnu"
 ; large_indirect_bar_alias is visible to main.ll but its aliasee isn't.
 @calleeAddrs2 = global [1 x ptr] [ptr @large_indirect_bar_alias]
 
+; @read_write_global_vars is not read-only nor write-only (in main.ll). It's not
+; a constant global var and has references, so it's not importable as a definition.
+@read_write_global_vars = dso_local global [1 x ptr] [ptr @large_indirect_callee]
+
 define void @callee() #1 {
   ret void
 }
@@ -177,8 +207,12 @@ define void @large_indirect_bar()#2 {
 
 define internal void @small_indirect_callee() #0 {
 entry:
-  %0 = load ptr, ptr @calleeAddrs2
-  call void %0(), !prof !3
+  %0 = load ptr, ptr @calleeAddrs
+  ; The function-import pass crash (see pr/117584) when alias is imported but
+  ; aliasee isn't.
+  ; TODO: Update !prof to !3 (for @large_indirect_bar_alias) after alias/aliasee
+  ; import issue is handled.
+  call void %0(), !prof !0
   ret void
 }
 
@@ -197,6 +231,8 @@ entry:
   ret void
 }
 
+declare void @external_func()
+
 define void @large_func() #0 {
 entry:
   call void @callee()

[teresajohnson]

Are you trying to simulate a distributed ThinLTO backend? Add "-import-all-index" to the invocation, so that we don't try to recompute the import decisions which isn't what you want to do. This should fix the alias issue mentioned further below.

[mingmingl-llvm]

Are you trying to simulate a distributed ThinLTO backend? Add "-import-all-index" to the invocation, so that we don't try to recompute the import decisions which isn't what you want to do. This should fix the alias issue mentioned further below.

Thanks! I updated #117584 and this patch. PTAL.

[teresajohnson]

lgtm