[llvm] [ThinLTO]Supports declaration import for global variables in distributed ThinLTO (PR #117616)

Mingming Liu via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 2 14:36:05 PST 2024


https://github.com/mingmingl-llvm updated https://github.com/llvm/llvm-project/pull/117616

>From ea40919ce51307b6c96d0eb7ef8c1aa6bd04e60b Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Sun, 24 Nov 2024 21:32:44 -0800
Subject: [PATCH 1/3] [ThinLTO]Import global variable declaration

---
 llvm/include/llvm/IR/ModuleSummaryIndex.h     |  5 ++
 llvm/lib/Bitcode/Writer/BitcodeWriter.cpp     |  3 +-
 llvm/lib/IR/ModuleSummaryIndex.cpp            | 18 +++++++-
 llvm/lib/Transforms/IPO/FunctionImport.cpp    | 12 ++++-
 .../ThinLTO/X86/import_callee_declaration.ll  | 46 ++++++++++++++++---
 5 files changed, 73 insertions(+), 11 deletions(-)

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..d11331f1275814 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,25 @@
 ; 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
+
+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 +99,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,15 +119,16 @@
 ; 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
 
 ; RUN: llvm-dis in-process.2.2.internalize.bc -o - | FileCheck %s --check-prefix=INTERNALIZE
 
+; IMPORT-DAG: @read_write_global_vars = external dso_local global [1 x ptr]
 ; IMPORT-DAG: define available_externally void @small_func
 ; IMPORT-DAG: define available_externally hidden void @small_indirect_callee
 ; IMPORT-DAG: declare void @large_func
@@ -126,9 +143,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"
 
+ at 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 +159,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 +173,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.
+ at read_write_global_vars = dso_local global [1 x ptr] [ptr @large_indirect_callee]
+
 define void @callee() #1 {
   ret void
 }
@@ -177,8 +205,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 +229,8 @@ entry:
   ret void
 }
 
+declare void @external_func()
+
 define void @large_func() #0 {
 entry:
   call void @callee()

>From e658fde5bd44136fc00edeabeba9b49703391d41 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Mon, 25 Nov 2024 11:19:18 -0800
Subject: [PATCH 2/3] update test comments

---
 llvm/test/ThinLTO/X86/import_callee_declaration.ll | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/llvm/test/ThinLTO/X86/import_callee_declaration.ll b/llvm/test/ThinLTO/X86/import_callee_declaration.ll
index d11331f1275814..fd62aeb3f74401 100644
--- a/llvm/test/ThinLTO/X86/import_callee_declaration.ll
+++ b/llvm/test/ThinLTO/X86/import_callee_declaration.ll
@@ -78,6 +78,7 @@
 ; 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
@@ -128,10 +129,11 @@ MAIN-IMPORT: @read_write_global_vars = external dso_local global [1 x ptr]
 
 ; RUN: llvm-dis in-process.2.2.internalize.bc -o - | FileCheck %s --check-prefix=INTERNALIZE
 
-; IMPORT-DAG: @read_write_global_vars = external dso_local global [1 x ptr]
 ; 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

>From 1482742e1821a3e30ae242550a2e1871ef5fc963 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Mon, 2 Dec 2024 14:35:09 -0800
Subject: [PATCH 3/3] Simulate backend function-import correctly by adding
 option -import-all-index and update test case import_callee_declaration

---
 llvm/test/ThinLTO/X86/import_callee_declaration.ll | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/llvm/test/ThinLTO/X86/import_callee_declaration.ll b/llvm/test/ThinLTO/X86/import_callee_declaration.ll
index fd62aeb3f74401..c236e70569f538 100644
--- a/llvm/test/ThinLTO/X86/import_callee_declaration.ll
+++ b/llvm/test/ThinLTO/X86/import_callee_declaration.ll
@@ -50,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 3 function declarations imported from lib.bc
+; DUMP: - 2 function definitions and 4 function declarations imported from lib.bc
 
 ; First disassemble per-module summary and find out the GUID for {large_func, large_indirect_callee}.
 ;
@@ -71,11 +71,11 @@
 ; 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-NOT: gv: (guid: 13590951773474913315, summaries: (alias: (module: [[LIBMOD]], flags: ({{.*}} importType: declaration), aliasee: null)))
+; MAIN-DIS: 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: opt -passes=function-import -import-all-index -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.
@@ -122,7 +122,7 @@ MAIN-IMPORT: @read_write_global_vars = external dso_local global [1 x ptr]
 ; IMPORTDUMP-DAG: Is importing function declaration 2418497564662708935 large_func 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: Not importing alias 13590951773474913315 large_indirect_bar_alias from lib.cc
+; IMPORTDUMP-DAG: Is importing alias declaration 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
@@ -208,11 +208,7 @@ define void @large_indirect_bar()#2 {
 define internal void @small_indirect_callee() #0 {
 entry:
   %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
+  call void %0(), !prof !3
   ret void
 }
 



More information about the llvm-commits mailing list