From ea40919ce51307b6c96d0eb7ef8c1aa6bd04e60b Mon Sep 17 00:00:00 2001 From: mingmingl Date: Sun, 24 Nov 2024 21:32:44 -0800 Subject: [PATCH 1/5] [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 39c60229aa1d8..259391cbb718b 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 24a4c2e8303d5..dd6b97d521c4a 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 12a558b3bc1b1..d9024b0a8673f 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(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 fee27f72f208b..ca15a4b9e1be8 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 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 246920e5db0dc..d11331f127581 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" +@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. +@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 Date: Mon, 25 Nov 2024 11:19:18 -0800 Subject: [PATCH 2/5] 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 d11331f127581..fd62aeb3f7440 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 Date: Mon, 2 Dec 2024 14:35:09 -0800 Subject: [PATCH 3/5] 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 fd62aeb3f7440..c236e70569f53 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 } From f479450b94dd1a46380f86f2cc16bb6f0aef082c Mon Sep 17 00:00:00 2001 From: mingmingl Date: Mon, 2 Dec 2024 14:51:27 -0800 Subject: [PATCH 4/5] remove external_func decl in lib.cc to simplify test --- llvm/test/ThinLTO/X86/import_callee_declaration.ll | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/llvm/test/ThinLTO/X86/import_callee_declaration.ll b/llvm/test/ThinLTO/X86/import_callee_declaration.ll index c236e70569f53..07cb43c0f6a53 100644 --- a/llvm/test/ThinLTO/X86/import_callee_declaration.ll +++ b/llvm/test/ThinLTO/X86/import_callee_declaration.ll @@ -42,7 +42,6 @@ ; 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 @@ -108,7 +107,6 @@ MAIN-IMPORT: @read_write_global_vars = external dso_local global [1 x ptr] ; 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 @@ -207,7 +205,7 @@ define void @large_indirect_bar()#2 { define internal void @small_indirect_callee() #0 { entry: - %0 = load ptr, ptr @calleeAddrs + %0 = load ptr, ptr @calleeAddrs2 call void %0(), !prof !3 ret void } @@ -227,7 +225,7 @@ entry: ret void } -declare void @external_func() +;declare void @external_func() define void @large_func() #0 { entry: From da70c8e0881c60f6901f1c7676ff4b36ba3c0158 Mon Sep 17 00:00:00 2001 From: mingmingl Date: Mon, 2 Dec 2024 15:03:42 -0800 Subject: [PATCH 5/5] one-liner to clean up a comment to remove unused function decl --- llvm/test/ThinLTO/X86/import_callee_declaration.ll | 2 -- 1 file changed, 2 deletions(-) diff --git a/llvm/test/ThinLTO/X86/import_callee_declaration.ll b/llvm/test/ThinLTO/X86/import_callee_declaration.ll index 07cb43c0f6a53..72550fa4d6f0b 100644 --- a/llvm/test/ThinLTO/X86/import_callee_declaration.ll +++ b/llvm/test/ThinLTO/X86/import_callee_declaration.ll @@ -225,8 +225,6 @@ entry: ret void } -;declare void @external_func() - define void @large_func() #0 { entry: call void @callee()