[compiler-rt] [llvm] Fix buildbot failure by changing pointer type to GlobalValueSummary (PR #100508)
Mingming Liu via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 24 21:20:21 PDT 2024
https://github.com/minglotus-6 updated https://github.com/llvm/llvm-project/pull/100508
>From 1be94de97494770f898cb8febfd3e9698a97aa6a Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Tue, 23 Jul 2024 10:13:02 -0700
Subject: [PATCH 01/10] [InstrFDO]Import assume unique local
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 038785114a0cf..2e20f87400e1d 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -174,6 +174,11 @@ static cl::opt<std::string> WorkloadDefinitions(
"}"),
cl::Hidden);
+static cl::opt<bool> ImportAssumeUniqueLocal(
+ "import-assume-unique-local", cl::init(false),
+ cl::description("assume local-linkage global variables have unique names"),
+ cl::Hidden);
+
namespace llvm {
extern cl::opt<bool> EnableMemProfContextDisambiguation;
}
@@ -367,6 +372,8 @@ class GlobalsImporter final {
// was not distinguishing path.
auto LocalNotInModule =
[&](const GlobalValueSummary *RefSummary) -> bool {
+ if (ImportAssumeUniqueLocal)
+ return false;
return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
RefSummary->modulePath() != Summary.modulePath();
};
>From 78fe701864317f6e46d673beb46af5acf69147a3 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 11:38:10 -0700
Subject: [PATCH 02/10] update test case
---
.../Linux/instrprof-vtable-value-prof.cpp | 30 ++++++++++++++-----
llvm/lib/Transforms/IPO/FunctionImport.cpp | 9 +++++-
2 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
index 411640fc5176d..4ef7b2da2f572 100644
--- a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
+++ b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
@@ -1,3 +1,4 @@
+
// REQUIRES: lld, lld-available
// Building the instrumented binary will fail because lld doesn't support
@@ -5,7 +6,7 @@
// ld.lld: error: /lib/../lib64/Scrt1.o: ABI version 1 is not supported
// UNSUPPORTED: ppc && host-byteorder-big-endian
-// RUN: rm -rf %t && mkdir %t && cd %t
+// RUN: rm -rf %t && split-file %s %t && cd %t
// RUN: %clangxx_pgogen -fuse-ld=lld -O2 -fprofile-generate=. -mllvm -enable-vtable-value-profiling %s -o test
// RUN: env LLVM_PROFILE_FILE=test.profraw ./test
@@ -141,18 +142,19 @@
// RUN: -g -flto=thin -fwhole-program-vtables -fno-split-lto-unit -O2 \
// RUN: -mllvm -enable-vtable-value-profiling -Wl,-mllvm,-enable-vtable-value-profiling \
// RUN: -mllvm -enable-vtable-profile-use \
+// RUN: -Wl,-plugin-opt,-import-assume-unique-local \
// RUN: -Wl,-mllvm,-enable-vtable-profile-use -Rpass=pgo-icall-prom \
// RUN: -Wl,-mllvm,-print-after=pgo-icall-prom \
// RUN: -Wl,-mllvm,-filter-print-funcs=main %s 2>&1 \
// RUN: | FileCheck %s --check-prefixes=REMARK,IR --implicit-check-not="!VP"
// For the indirect call site `ptr->func`
-// REMARK: instrprof-vtable-value-prof.cpp:226:19: Promote indirect call to _ZN12_GLOBAL__N_18Derived24funcEii with count 150 out of 200, sink 1 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
-// REMARK: instrprof-vtable-value-prof.cpp:226:19: Promote indirect call to _ZN8Derived14funcEii with count 50 out of 50, sink 1 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
+// REMARK: instrprof-vtable-value-prof.cpp:240:19: Promote indirect call to _ZN12_GLOBAL__N_18Derived24funcEii with count 150 out of 200, sink 1 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
+// REMARK: instrprof-vtable-value-prof.cpp:240:19: Promote indirect call to _ZN8Derived14funcEii with count 50 out of 50, sink 1 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
//
// For the indirect call site `delete ptr`
-// REMARK: instrprof-vtable-value-prof.cpp:228:5: Promote indirect call to _ZN12_GLOBAL__N_18Derived2D0Ev with count 750 out of 1000, sink 2 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
-// REMARK: instrprof-vtable-value-prof.cpp:228:5: Promote indirect call to _ZN8Derived1D0Ev with count 250 out of 250, sink 2 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
+// REMARK: instrprof-vtable-value-prof.cpp:242:5: Promote indirect call to _ZN12_GLOBAL__N_18Derived2D0Ev with count 750 out of 1000, sink 2 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
+// REMARK: instrprof-vtable-value-prof.cpp:242:5: Promote indirect call to _ZN8Derived1D0Ev with count 250 out of 250, sink 2 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
// The IR matchers for indirect callsite `ptr->func`.
// IR-LABEL: @main
@@ -185,6 +187,7 @@
// IR: [[MERGE0]]:
// IR: [[RES2:%.*]] = phi i32 [ [[RES1]], %[[MERGE1]] ], [ [[RESBB1]], %[[BB1]] ]
+//--- lib.h
#include <stdio.h>
#include <stdlib.h>
class Base {
@@ -193,21 +196,31 @@ class Base {
virtual ~Base() {};
};
+
class Derived1 : public Base {
public:
- int func(int a, int b) override { return a * b; }
+ int func(int a, int b) override;
~Derived1() {}
};
namespace {
class Derived2 : public Base {
public:
- int func(int a, int b) override { return a * (a - b); }
+ int func(int a, int b) override;
~Derived2() {}
};
} // namespace
-__attribute__((noinline)) Base *createType(int a) {
+
+__attribute__((noinline)) Base *createType(int a);
+
+//-- lib.cpp
+
+int Derived1::func(int a, int b) { return a * b; }
+
+int Derived2::func(int a, int b) { return a * (a - b); }
+
+Base *createType(int a) {
Base *base = nullptr;
if (a % 4 == 0)
base = new Derived1();
@@ -216,6 +229,7 @@ __attribute__((noinline)) Base *createType(int a) {
return base;
}
+//--- main.cpp
int main(int argc, char **argv) {
int sum = 0;
for (int i = 0; i < 1000; i++) {
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 2e20f87400e1d..839c09fa9d586 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -176,7 +176,14 @@ static cl::opt<std::string> WorkloadDefinitions(
static cl::opt<bool> ImportAssumeUniqueLocal(
"import-assume-unique-local", cl::init(false),
- cl::description("assume local-linkage global variables have unique names"),
+ cl::desc(
+ "By default, a local-linkage global variable won't be imported in the "
+ "edge mod1:func -> mod2:func_bar -> mod2:local-var since compiler "
+ "cannot assume mod2 is compiled with full path or local-var has a "
+ "unique GUID. "
+ "Set this option to true will help cross-module import of such "
+ "variables. But it is only safe if the compiler user specify the full "
+ "module path."),
cl::Hidden);
namespace llvm {
>From d47ce4e4d56756f54ff0ed18b7f3269ff9d278ae Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 12:34:31 -0700
Subject: [PATCH 03/10] fix test to make it pass at HEAD
---
.../Linux/instrprof-vtable-value-prof.cpp | 62 ++++++++++---------
llvm/lib/Transforms/IPO/FunctionImport.cpp | 4 ++
2 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
index 4ef7b2da2f572..1d8a0f470310c 100644
--- a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
+++ b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
@@ -6,9 +6,9 @@
// ld.lld: error: /lib/../lib64/Scrt1.o: ABI version 1 is not supported
// UNSUPPORTED: ppc && host-byteorder-big-endian
-// RUN: rm -rf %t && split-file %s %t && cd %t
+// RUN: rm -rf %t && mkdir %t && split-file %s %t && cd %t
-// RUN: %clangxx_pgogen -fuse-ld=lld -O2 -fprofile-generate=. -mllvm -enable-vtable-value-profiling %s -o test
+// RUN: %clangxx_pgogen -fuse-ld=lld -O2 -fprofile-generate=. -mllvm -enable-vtable-value-profiling lib.cpp main.cpp -o test
// RUN: env LLVM_PROFILE_FILE=test.profraw ./test
// Show vtable profiles from raw profile.
@@ -38,23 +38,23 @@
// COMMON-NEXT: Number of instrumented vtables: 2
// RAW: Indirect Target Results:
// RAW-NEXT: [ 0, _ZN8Derived14funcEii, 50 ] (25.00%)
-// RAW-NEXT: [ 0, {{.*}}instrprof-vtable-value-prof.cpp;_ZN12_GLOBAL__N_18Derived24funcEii, 150 ] (75.00%)
+// RAW-NEXT: [ 0, {{.*}}lib.cpp;_ZN12_GLOBAL__N_18Derived24funcEii, 150 ] (75.00%)
// RAW-NEXT: [ 1, _ZN8Derived1D0Ev, 250 ] (25.00%)
-// RAW-NEXT: [ 1, {{.*}}instrprof-vtable-value-prof.cpp;_ZN12_GLOBAL__N_18Derived2D0Ev, 750 ] (75.00%)
+// RAW-NEXT: [ 1, {{.*}}lib.cpp;_ZN12_GLOBAL__N_18Derived2D0Ev, 750 ] (75.00%)
// RAW-NEXT: VTable Results:
// RAW-NEXT: [ 0, _ZTV8Derived1, 50 ] (25.00%)
-// RAW-NEXT: [ 0, {{.*}}instrprof-vtable-value-prof.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 150 ] (75.00%)
+// RAW-NEXT: [ 0, {{.*}}lib.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 150 ] (75.00%)
// RAW-NEXT: [ 1, _ZTV8Derived1, 250 ] (25.00%)
-// RAW-NEXT: [ 1, {{.*}}instrprof-vtable-value-prof.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 750 ] (75.00%)
+// RAW-NEXT: [ 1, {{.*}}lib.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 750 ] (75.00%)
// INDEXED: Indirect Target Results:
-// INDEXED-NEXT: [ 0, {{.*}}instrprof-vtable-value-prof.cpp;_ZN12_GLOBAL__N_18Derived24funcEii, 150 ] (75.00%)
+// INDEXED-NEXT: [ 0, {{.*}}lib.cpp;_ZN12_GLOBAL__N_18Derived24funcEii, 150 ] (75.00%)
// INDEXED-NEXT: [ 0, _ZN8Derived14funcEii, 50 ] (25.00%)
-// INDEXED-NEXT: [ 1, {{.*}}instrprof-vtable-value-prof.cpp;_ZN12_GLOBAL__N_18Derived2D0Ev, 750 ] (75.00%)
+// INDEXED-NEXT: [ 1, {{.*}}lib.cpp;_ZN12_GLOBAL__N_18Derived2D0Ev, 750 ] (75.00%)
// INDEXED-NEXT: [ 1, _ZN8Derived1D0Ev, 250 ] (25.00%)
// INDEXED-NEXT: VTable Results:
-// INDEXED-NEXT: [ 0, {{.*}}instrprof-vtable-value-prof.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 150 ] (75.00%)
+// INDEXED-NEXT: [ 0, {{.*}}lib.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 150 ] (75.00%)
// INDEXED-NEXT: [ 0, _ZTV8Derived1, 50 ] (25.00%)
-// INDEXED-NEXT: [ 1, {{.*}}instrprof-vtable-value-prof.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 750 ] (75.00%)
+// INDEXED-NEXT: [ 1, {{.*}}lib.cpp;_ZTVN12_GLOBAL__N_18Derived2E, 750 ] (75.00%)
// INDEXED-NEXT: [ 1, _ZTV8Derived1, 250 ] (25.00%)
// COMMON: Instrumentation level: IR entry_first = 0
// COMMON-NEXT: Functions shown: 1
@@ -94,27 +94,27 @@
// ICTEXT: # NumValueSites:
// ICTEXT: 2
// ICTEXT: 2
-// ICTEXT: {{.*}}instrprof-vtable-value-prof.cpp;_ZN12_GLOBAL__N_18Derived24funcEii:150
+// ICTEXT: {{.*}}lib.cpp;_ZN12_GLOBAL__N_18Derived24funcEii:150
// ICTEXT: _ZN8Derived14funcEii:50
// ICTEXT: 2
-// ICTEXT: {{.*}}instrprof-vtable-value-prof.cpp;_ZN12_GLOBAL__N_18Derived2D0Ev:750
+// ICTEXT: {{.*}}lib.cpp;_ZN12_GLOBAL__N_18Derived2D0Ev:750
// ICTEXT: _ZN8Derived1D0Ev:250
// ICTEXT: # ValueKind = IPVK_VTableTarget:
// ICTEXT: 2
// ICTEXT: # NumValueSites:
// ICTEXT: 2
// ICTEXT: 2
-// ICTEXT: {{.*}}instrprof-vtable-value-prof.cpp;_ZTVN12_GLOBAL__N_18Derived2E:150
+// ICTEXT: {{.*}}lib.cpp;_ZTVN12_GLOBAL__N_18Derived2E:150
// ICTEXT: _ZTV8Derived1:50
// ICTEXT: 2
-// ICTEXT: {{.*}}instrprof-vtable-value-prof.cpp;_ZTVN12_GLOBAL__N_18Derived2E:750
+// ICTEXT: {{.*}}lib.cpp;_ZTVN12_GLOBAL__N_18Derived2E:750
// ICTEXT: _ZTV8Derived1:250
// When vtable value profiles exist, pgo-instr-use pass should annotate them
// even if `-enable-vtable-value-profiling` is not explicitly on.
// RUN: %clangxx -m64 -fprofile-use=test.profdata -fuse-ld=lld -O2 \
// RUN: -mllvm -print-after=pgo-instr-use -mllvm -filter-print-funcs=main \
-// RUN: -mllvm -print-module-scope %s 2>&1 | FileCheck %s --check-prefix=ANNOTATE
+// RUN: -mllvm -print-module-scope lib.cpp main.cpp 2>&1 | FileCheck %s --check-prefix=ANNOTATE
// ANNOTATE-NOT: Inconsistent number of value sites
// ANNOTATE: !{!"VP", i32 2
@@ -123,7 +123,7 @@
// if `-icp-max-num-vtables` is set to zero.
// RUN: %clangxx -m64 -fprofile-use=test.profdata -fuse-ld=lld -O2 \
// RUN: -mllvm -icp-max-num-vtables=0 -mllvm -print-after=pgo-instr-use \
-// RUN: -mllvm -filter-print-funcs=main -mllvm -print-module-scope %s 2>&1 | \
+// RUN: -mllvm -filter-print-funcs=main -mllvm -print-module-scope lib.cpp main.cpp 2>&1 | \
// RUN: FileCheck %s --check-prefix=OMIT
// OMIT: Inconsistent number of value sites
@@ -145,26 +145,26 @@
// RUN: -Wl,-plugin-opt,-import-assume-unique-local \
// RUN: -Wl,-mllvm,-enable-vtable-profile-use -Rpass=pgo-icall-prom \
// RUN: -Wl,-mllvm,-print-after=pgo-icall-prom \
-// RUN: -Wl,-mllvm,-filter-print-funcs=main %s 2>&1 \
+// RUN: -Wl,-mllvm,-filter-print-funcs=main lib.cpp main.cpp 2>&1 \
// RUN: | FileCheck %s --check-prefixes=REMARK,IR --implicit-check-not="!VP"
// For the indirect call site `ptr->func`
-// REMARK: instrprof-vtable-value-prof.cpp:240:19: Promote indirect call to _ZN12_GLOBAL__N_18Derived24funcEii with count 150 out of 200, sink 1 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
-// REMARK: instrprof-vtable-value-prof.cpp:240:19: Promote indirect call to _ZN8Derived14funcEii with count 50 out of 50, sink 1 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
+// REMARK: main.cpp:10:19: Promote indirect call to _ZN12_GLOBAL__N_18Derived24funcEii.llvm.{{.*}} with count 150 out of 200, sink 1 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E.llvm.{{.*}}}
+// REMARK: main.cpp:10:19: Promote indirect call to _ZN8Derived14funcEii with count 50 out of 50, sink 1 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
//
// For the indirect call site `delete ptr`
-// REMARK: instrprof-vtable-value-prof.cpp:242:5: Promote indirect call to _ZN12_GLOBAL__N_18Derived2D0Ev with count 750 out of 1000, sink 2 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
-// REMARK: instrprof-vtable-value-prof.cpp:242:5: Promote indirect call to _ZN8Derived1D0Ev with count 250 out of 250, sink 2 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
+// REMARK: main.cpp:12:5: Promote indirect call to _ZN12_GLOBAL__N_18Derived2D0Ev.llvm.{{.*}} with count 750 out of 1000, sink 2 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E.llvm.{{.*}}}
+// REMARK: main.cpp:12:5: Promote indirect call to _ZN8Derived1D0Ev with count 250 out of 250, sink 2 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
// The IR matchers for indirect callsite `ptr->func`.
// IR-LABEL: @main
// IR: [[OBJ:%.*]] = {{.*}}call {{.*}} @_Z10createTypei
// IR: [[VTABLE:%.*]] = load ptr, ptr [[OBJ]]
-// IR: [[CMP1:%.*]] = icmp eq ptr [[VTABLE]], getelementptr inbounds (i8, ptr @_ZTVN12_GLOBAL__N_18Derived2E, i32 16)
+// IR: [[CMP1:%.*]] = icmp eq ptr [[VTABLE]], getelementptr inbounds (i8, ptr @_ZTVN12_GLOBAL__N_18Derived2E.llvm.{{.*}}, i32 16)
// IR: br i1 [[CMP1]], label %[[BB1:.*]], label %[[BB2:[a-zA-Z0-9_.]+]],
//
// IR: [[BB1]]:
-// IR: [[RESBB1:%.*]] = {{.*}}call {{.*}} @_ZN12_GLOBAL__N_18Derived24funcEii
+// IR: [[RESBB1:%.*]] = {{.*}}call {{.*}} @_ZN12_GLOBAL__N_18Derived24funcEii.llvm.{{.*}}
// IR: br label %[[MERGE0:[a-zA-Z0-9_.]+]]
//
// IR: [[BB2]]:
@@ -203,23 +203,23 @@ class Derived1 : public Base {
~Derived1() {}
};
+
+__attribute__((noinline)) Base *createType(int a);
+
+//--- lib.cpp
+#include "lib.h"
+
namespace {
class Derived2 : public Base {
public:
- int func(int a, int b) override;
+ int func(int a, int b) override { return a * (a - b); }
~Derived2() {}
};
} // namespace
-__attribute__((noinline)) Base *createType(int a);
-
-//-- lib.cpp
-
int Derived1::func(int a, int b) { return a * b; }
-int Derived2::func(int a, int b) { return a * (a - b); }
-
Base *createType(int a) {
Base *base = nullptr;
if (a % 4 == 0)
@@ -230,6 +230,8 @@ Base *createType(int a) {
}
//--- main.cpp
+#include "lib.h"
+
int main(int argc, char **argv) {
int sum = 0;
for (int i = 0; i < 1000; i++) {
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 839c09fa9d586..08976058fb592 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -381,6 +381,10 @@ class GlobalsImporter final {
[&](const GlobalValueSummary *RefSummary) -> bool {
if (ImportAssumeUniqueLocal)
return false;
+
+ errs() << "Ref VI is " << VI << "\n";
+ errs() << RefSummary->modulePath() << "\n";
+ errs() << Summary.modulePath() << "\n";
return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
RefSummary->modulePath() != Summary.modulePath();
};
>From eff8695ba4fdc43b29eaea5343c7cec71823799a Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 12:45:21 -0700
Subject: [PATCH 04/10] remove debuging logs
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 3 ---
1 file changed, 3 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 08976058fb592..3bbdac667198b 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -382,9 +382,6 @@ class GlobalsImporter final {
if (ImportAssumeUniqueLocal)
return false;
- errs() << "Ref VI is " << VI << "\n";
- errs() << RefSummary->modulePath() << "\n";
- errs() << Summary.modulePath() << "\n";
return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
RefSummary->modulePath() != Summary.modulePath();
};
>From 7486f9d6121850efd0a47f3722799560c1b9dc3a Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 14:14:19 -0700
Subject: [PATCH 05/10] handle indirect callee and indirect ref vtable
consistently
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 51 +++++++++++-----------
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 3bbdac667198b..af67b070f1853 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -178,12 +178,11 @@ static cl::opt<bool> ImportAssumeUniqueLocal(
"import-assume-unique-local", cl::init(false),
cl::desc(
"By default, a local-linkage global variable won't be imported in the "
- "edge mod1:func -> mod2:func_bar -> mod2:local-var since compiler "
- "cannot assume mod2 is compiled with full path or local-var has a "
- "unique GUID. "
- "Set this option to true will help cross-module import of such "
- "variables. But it is only safe if the compiler user specify the full "
- "module path."),
+ "edge mod1:func -> mod2:local-var (from value profiles) since compiler "
+ "cannot assume mod2 is compiled with full path which gives local-var a "
+ "program-wide unique GUID. Set this option to true will help cross "
+ "module import of such variables. This is only safe if the compiler "
+ "user specify the full module path."),
cl::Hidden);
namespace llvm {
@@ -208,6 +207,23 @@ static std::unique_ptr<Module> loadFile(const std::string &FileName,
return Result;
}
+static shouldSkipLocalInAnotherModule(const GlobalVarSummary *RefSummary,
+ size_t NumRefs,
+ StringRef ImporterModule) {
+ // We can import a local from another module if all inputs are compiled
+ // with full paths or when there is one entry in the list.
+ if (ImportAssumeUniqueLocal || NumRefs == 1)
+ return false;
+ // If this is a local variable, make sure we import the copy
+ // in the caller's module. The only time a local variable can
+ // share an entry in the index is if there is a local with the same name
+ // in another module that had the same source file name (in a different
+ // directory), where each was compiled in their own directory so there
+ // was not distinguishing path.
+ return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
+ RefSummary->modulePath() != ImporterModule;
+}
+
/// Given a list of possible callee implementation for a call site, qualify the
/// legality of importing each. The return is a range of pairs. Each pair
/// corresponds to a candidate. The first value is the ImportFailureReason for
@@ -250,9 +266,8 @@ static auto qualifyCalleeCandidates(
// entry in the list - in that case this must be a reference due
// to indirect call profile data, since a function pointer can point to
// a local in another module.
- if (GlobalValue::isLocalLinkage(Summary->linkage()) &&
- CalleeSummaryList.size() > 1 &&
- Summary->modulePath() != CallerModulePath)
+ if (shouldSkipLocalInAnotherModule(Summary, CalleeSummaryList.size(),
+ CallerModulePath))
return {
FunctionImporter::ImportFailureReason::LocalLinkageNotInModule,
GVSummary};
@@ -371,21 +386,6 @@ class GlobalsImporter final {
LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");
- // If this is a local variable, make sure we import the copy
- // in the caller's module. The only time a local variable can
- // share an entry in the index is if there is a local with the same name
- // in another module that had the same source file name (in a different
- // directory), where each was compiled in their own directory so there
- // was not distinguishing path.
- auto LocalNotInModule =
- [&](const GlobalValueSummary *RefSummary) -> bool {
- if (ImportAssumeUniqueLocal)
- return false;
-
- return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
- RefSummary->modulePath() != Summary.modulePath();
- };
-
for (const auto &RefSummary : VI.getSummaryList()) {
const auto *GVS = dyn_cast<GlobalVarSummary>(RefSummary.get());
// Functions could be referenced by global vars - e.g. a vtable; but we
@@ -394,7 +394,8 @@ class GlobalsImporter final {
// 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) ||
- LocalNotInModule(GVS))
+ shouldSkipLocalInAnotherModule(GVS, VI.getSummaryList().size(),
+ Summary.modulePath()))
continue;
// If there isn't an entry for GUID, insert <GUID, Definition> pair.
>From 5163477b4041097dea7383a9130d26db5120a2aa Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 14:23:11 -0700
Subject: [PATCH 06/10] minor fixes
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 43 ++++++++++++----------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index af67b070f1853..74d2c2816f6e5 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -207,19 +207,19 @@ static std::unique_ptr<Module> loadFile(const std::string &FileName,
return Result;
}
-static shouldSkipLocalInAnotherModule(const GlobalVarSummary *RefSummary,
- size_t NumRefs,
- StringRef ImporterModule) {
+static bool shouldSkipLocalInAnotherModule(const GlobalVarSummary *RefSummary,
+ size_t NumRefs,
+ StringRef ImporterModule) {
// We can import a local from another module if all inputs are compiled
// with full paths or when there is one entry in the list.
if (ImportAssumeUniqueLocal || NumRefs == 1)
return false;
- // If this is a local variable, make sure we import the copy
- // in the caller's module. The only time a local variable can
- // share an entry in the index is if there is a local with the same name
- // in another module that had the same source file name (in a different
- // directory), where each was compiled in their own directory so there
- // was not distinguishing path.
+ // In other cases, make sure we import the copy in the caller's module if the
+ // referenced value has local linkage. The only time a local variable can
+ // share an entry in the index is if there is a local with the same name in
+ // another module that had the same source file name (in a different
+ // directory), where each was compiled in their own directory so there was not
+ // distinguishing path.
return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
RefSummary->modulePath() != ImporterModule;
}
@@ -256,17 +256,20 @@ static auto qualifyCalleeCandidates(
if (!Summary)
return {FunctionImporter::ImportFailureReason::GlobalVar, GVSummary};
- // If this is a local function, make sure we import the copy
- // in the caller's module. The only time a local function can
- // share an entry in the index is if there is a local with the same name
- // in another module that had the same source file name (in a different
- // directory), where each was compiled in their own directory so there
- // was not distinguishing path.
- // However, do the import from another module if there is only one
- // entry in the list - in that case this must be a reference due
- // to indirect call profile data, since a function pointer can point to
- // a local in another module.
- if (shouldSkipLocalInAnotherModule(Summary, CalleeSummaryList.size(),
+ // If this is a local function, make sure we import the copy in the
+ // caller's module. The only time a local function can share an entry in
+ // the index is if there is a local with the same name in another module
+ // that had the same source file name (in a different directory), where
+ // each was compiled in their own directory so there was not
+ // distinguishing path.
+ // If the local function is from another module, it must be a reference
+ // due to indirect call profile data since a function pointer can point
+ // to a local in another module. Do the import from another module if
+ // there is only one entry in the list or when all files in the program
+ // are compiled with full path - in both cases the local function has
+ // unique PGO name and GUID.
+ if (shouldSkipLocalInAnotherModule(dyn_cast<GlobalVarSummary>(Summary),
+ CalleeSummaryList.size(),
CallerModulePath))
return {
FunctionImporter::ImportFailureReason::LocalLinkageNotInModule,
>From c677c575ccc1fadbe8aba72378b1ebb9056b439b Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 18:14:13 -0700
Subject: [PATCH 07/10] resolve review feedback
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 74d2c2816f6e5..547433c280869 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -208,11 +208,11 @@ static std::unique_ptr<Module> loadFile(const std::string &FileName,
}
static bool shouldSkipLocalInAnotherModule(const GlobalVarSummary *RefSummary,
- size_t NumRefs,
+ size_t NumDefs,
StringRef ImporterModule) {
// We can import a local from another module if all inputs are compiled
- // with full paths or when there is one entry in the list.
- if (ImportAssumeUniqueLocal || NumRefs == 1)
+ // with full paths or when there is one definition.
+ if (ImportAssumeUniqueLocal || NumDefs == 1)
return false;
// In other cases, make sure we import the copy in the caller's module if the
// referenced value has local linkage. The only time a local variable can
>From d6ce694c36325baddd05bab421e65ecffde873c9 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 18:15:50 -0700
Subject: [PATCH 08/10] remove one blank line
---
compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
index 1d8a0f470310c..214c1504db7f4 100644
--- a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
+++ b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
@@ -1,4 +1,3 @@
-
// REQUIRES: lld, lld-available
// Building the instrumented binary will fail because lld doesn't support
>From 9df09b38cb8e9acc078968d3efd1224439f494c9 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 21:13:08 -0700
Subject: [PATCH 09/10] Fix crash PR 100448
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index 547433c280869..210b79e3a0adc 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -207,7 +207,7 @@ static std::unique_ptr<Module> loadFile(const std::string &FileName,
return Result;
}
-static bool shouldSkipLocalInAnotherModule(const GlobalVarSummary *RefSummary,
+static bool shouldSkipLocalInAnotherModule(const GlobalValueSummary *RefSummary,
size_t NumDefs,
StringRef ImporterModule) {
// We can import a local from another module if all inputs are compiled
@@ -268,8 +268,7 @@ static auto qualifyCalleeCandidates(
// there is only one entry in the list or when all files in the program
// are compiled with full path - in both cases the local function has
// unique PGO name and GUID.
- if (shouldSkipLocalInAnotherModule(dyn_cast<GlobalVarSummary>(Summary),
- CalleeSummaryList.size(),
+ if (shouldSkipLocalInAnotherModule(Summary, CalleeSummaryList.size(),
CallerModulePath))
return {
FunctionImporter::ImportFailureReason::LocalLinkageNotInModule,
>From e8a78b836b6f93b28d416a520a35d77a307048b3 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Wed, 24 Jul 2024 21:19:56 -0700
Subject: [PATCH 10/10] fix merge conflict
---
llvm/lib/Transforms/IPO/FunctionImport.cpp | 2 --
1 file changed, 2 deletions(-)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index e77bc9aa5ea17..74e64afeb22a6 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -207,9 +207,7 @@ static std::unique_ptr<Module> loadFile(const std::string &FileName,
return Result;
}
-<<<<<<< HEAD
static bool shouldSkipLocalInAnotherModule(const GlobalValueSummary *RefSummary,
-=======
static bool shouldSkipLocalInAnotherModule(const GlobalVarSummary *RefSummary,
>>>>>>> main
size_t NumDefs,
More information about the llvm-commits
mailing list