[clang] [llvm] [InstrPGO] Avoid using global variable to fix potential data race (PR #114364)
Lei Wang via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 31 10:00:17 PDT 2024
https://github.com/wlei-llvm updated https://github.com/llvm/llvm-project/pull/114364
>From a7b444bd75d6f83ed0f5692783990a59f36e8459 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Thu, 31 Oct 2024 09:58:27 -0700
Subject: [PATCH 1/3] Reapply "[InstrPGO] Support cold function coverage
instrumentation (#109837)"
This reverts commit d924a9ba03a05b417676e84f6c81aac44f907f71.
---
clang/include/clang/Driver/Options.td | 6 ++++
clang/lib/Driver/ToolChain.cpp | 4 ++-
clang/lib/Driver/ToolChains/Clang.cpp | 20 +++++++++++
.../test/CodeGen/pgo-cold-function-coverage.c | 19 ++++++++++
...fprofile-generate-cold-function-coverage.c | 8 +++++
llvm/lib/Passes/PassBuilderPipelines.cpp | 17 ++++++++-
.../Instrumentation/PGOInstrumentation.cpp | 19 ++++++++++
.../PGOProfile/instr-gen-cold-function.ll | 35 +++++++++++++++++++
8 files changed, 126 insertions(+), 2 deletions(-)
create mode 100644 clang/test/CodeGen/pgo-cold-function-coverage.c
create mode 100644 clang/test/Driver/fprofile-generate-cold-function-coverage.c
create mode 100644 llvm/test/Transforms/PGOProfile/instr-gen-cold-function.ll
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index c8bc2fe377b8ec..2814d2b1bf3733 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1786,6 +1786,12 @@ defm debug_info_for_profiling : BoolFOption<"debug-info-for-profiling",
PosFlag<SetTrue, [], [ClangOption, CC1Option],
"Emit extra debug info to make sample profile more accurate">,
NegFlag<SetFalse>>;
+def fprofile_generate_cold_function_coverage : Flag<["-"], "fprofile-generate-cold-function-coverage">,
+ Group<f_Group>, Visibility<[ClangOption, CLOption]>,
+ HelpText<"Generate instrumented code to collect coverage info for cold functions into default.profraw file (overridden by '=' form of option or LLVM_PROFILE_FILE env var)">;
+def fprofile_generate_cold_function_coverage_EQ : Joined<["-"], "fprofile-generate-cold-function-coverage=">,
+ Group<f_Group>, Visibility<[ClangOption, CLOption]>, MetaVarName<"<directory>">,
+ HelpText<"Generate instrumented code to collect coverage info for cold functions into <directory>/default.profraw (overridden by LLVM_PROFILE_FILE env var)">;
def fprofile_instr_generate : Flag<["-"], "fprofile-instr-generate">,
Group<f_Group>, Visibility<[ClangOption, CLOption]>,
HelpText<"Generate instrumented code to collect execution counts into default.profraw file (overridden by '=' form of option or LLVM_PROFILE_FILE env var)">;
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 6d3ede40691093..bdf3da0c96adca 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -899,7 +899,9 @@ bool ToolChain::needsProfileRT(const ArgList &Args) {
Args.hasArg(options::OPT_fprofile_instr_generate) ||
Args.hasArg(options::OPT_fprofile_instr_generate_EQ) ||
Args.hasArg(options::OPT_fcreate_profile) ||
- Args.hasArg(options::OPT_forder_file_instrumentation);
+ Args.hasArg(options::OPT_forder_file_instrumentation) ||
+ Args.hasArg(options::OPT_fprofile_generate_cold_function_coverage) ||
+ Args.hasArg(options::OPT_fprofile_generate_cold_function_coverage_EQ);
}
bool ToolChain::needsGCovInstrumentation(const llvm::opt::ArgList &Args) {
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 04b3832327a99c..4c6f508f1f24a6 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -632,6 +632,26 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
}
}
+ if (auto *ColdFuncCoverageArg = Args.getLastArg(
+ options::OPT_fprofile_generate_cold_function_coverage,
+ options::OPT_fprofile_generate_cold_function_coverage_EQ)) {
+ SmallString<128> Path(
+ ColdFuncCoverageArg->getOption().matches(
+ options::OPT_fprofile_generate_cold_function_coverage_EQ)
+ ? ColdFuncCoverageArg->getValue()
+ : "");
+ llvm::sys::path::append(Path, "default_%m.profraw");
+ // FIXME: Idealy the file path should be passed through
+ // `-fprofile-instrument-path=`(InstrProfileOutput), however, this field is
+ // shared with other profile use path(see PGOOptions), we need to refactor
+ // PGOOptions to make it work.
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back(Args.MakeArgString(
+ Twine("--instrument-cold-function-only-path=") + Path));
+ CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("--pgo-function-entry-coverage");
+ }
+
Arg *PGOGenArg = nullptr;
if (PGOGenerateArg) {
assert(!CSPGOGenerateArg);
diff --git a/clang/test/CodeGen/pgo-cold-function-coverage.c b/clang/test/CodeGen/pgo-cold-function-coverage.c
new file mode 100644
index 00000000000000..fd1e1e7e14cda5
--- /dev/null
+++ b/clang/test/CodeGen/pgo-cold-function-coverage.c
@@ -0,0 +1,19 @@
+// Test -fprofile-generate-cold-function-coverage
+
+// RUN: rm -rf %t && split-file %s %t
+// RUN: %clang -O2 -fprofile-generate-cold-function-coverage=/xxx/yyy/ -fprofile-sample-accurate -fprofile-sample-use=%t/pgo-cold-func.prof -S -emit-llvm -o - %t/pgo-cold-func.c | FileCheck %s
+
+// CHECK: @__llvm_profile_filename = {{.*}} c"/xxx/yyy/default_%m.profraw\00"
+
+// CHECK: store i8 0, ptr @__profc_bar, align 1
+// CHECK-NOT: @__profc_foo
+
+//--- pgo-cold-func.prof
+foo:1:1
+ 1: 1
+
+//--- pgo-cold-func.c
+int bar(int x) { return x;}
+int foo(int x) {
+ return x;
+}
diff --git a/clang/test/Driver/fprofile-generate-cold-function-coverage.c b/clang/test/Driver/fprofile-generate-cold-function-coverage.c
new file mode 100644
index 00000000000000..9b2f46423f34b1
--- /dev/null
+++ b/clang/test/Driver/fprofile-generate-cold-function-coverage.c
@@ -0,0 +1,8 @@
+// RUN: %clang -### -c -fprofile-generate-cold-function-coverage %s 2>&1 | FileCheck %s
+// CHECK: "--instrument-cold-function-only-path=default_%m.profraw"
+// CHECK: "--pgo-function-entry-coverage"
+// CHECK-NOT: "-fprofile-instrument"
+// CHECK-NOT: "-fprofile-instrument-path=
+
+// RUN: %clang -### -c -fprofile-generate-cold-function-coverage=dir %s 2>&1 | FileCheck %s --check-prefix=CHECK-EQ
+// CHECK-EQ: "--instrument-cold-function-only-path=dir{{/|\\\\}}default_%m.profraw"
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index 6478667e22b851..c391853c8d0c2b 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -296,7 +296,12 @@ static cl::opt<bool> UseLoopVersioningLICM(
"enable-loop-versioning-licm", cl::init(false), cl::Hidden,
cl::desc("Enable the experimental Loop Versioning LICM pass"));
+static cl::opt<std::string> InstrumentColdFuncOnlyPath(
+ "instrument-cold-function-only-path", cl::init(""),
+ cl::desc("File path for cold function only instrumentation"), cl::Hidden);
+
extern cl::opt<std::string> UseCtxProfile;
+extern cl::opt<bool> PGOInstrumentColdFunctionOnly;
namespace llvm {
extern cl::opt<bool> EnableMemProfContextDisambiguation;
@@ -1183,8 +1188,13 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
const bool IsCtxProfUse =
!UseCtxProfile.empty() && Phase == ThinOrFullLTOPhase::ThinLTOPreLink;
+ // Enable cold function coverage instrumentation if
+ // InstrumentColdFuncOnlyPath is provided.
+ const bool IsColdFuncOnlyInstrGen = PGOInstrumentColdFunctionOnly =
+ IsPGOPreLink && !InstrumentColdFuncOnlyPath.empty();
+
if (IsPGOInstrGen || IsPGOInstrUse || IsMemprofUse || IsCtxProfGen ||
- IsCtxProfUse)
+ IsCtxProfUse || IsColdFuncOnlyInstrGen)
addPreInlinerPasses(MPM, Level, Phase);
// Add all the requested passes for instrumentation PGO, if requested.
@@ -1206,6 +1216,11 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
return MPM;
addPostPGOLoopRotation(MPM, Level);
MPM.addPass(PGOCtxProfLoweringPass());
+ } else if (IsColdFuncOnlyInstrGen) {
+ addPGOInstrPasses(
+ MPM, Level, /* RunProfileGen */ true, /* IsCS */ false,
+ /* AtomicCounterUpdate */ false, InstrumentColdFuncOnlyPath,
+ /* ProfileRemappingFile */ "", IntrusiveRefCntPtr<vfs::FileSystem>());
}
if (IsPGOInstrGen || IsPGOInstrUse || IsCtxProfGen)
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index bceb6135cc1f92..4d8141431a0c19 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -319,6 +319,20 @@ static cl::opt<unsigned> PGOFunctionCriticalEdgeThreshold(
cl::desc("Do not instrument functions with the number of critical edges "
" greater than this threshold."));
+static cl::opt<uint64_t> PGOColdInstrumentEntryThreshold(
+ "pgo-cold-instrument-entry-threshold", cl::init(0), cl::Hidden,
+ cl::desc("For cold function instrumentation, skip instrumenting functions "
+ "whose entry count is above the given value."));
+
+static cl::opt<bool> PGOTreatUnknownAsCold(
+ "pgo-treat-unknown-as-cold", cl::init(false), cl::Hidden,
+ cl::desc("For cold function instrumentation, treat count unknown(e.g. "
+ "unprofiled) functions as cold."));
+
+cl::opt<bool> PGOInstrumentColdFunctionOnly(
+ "pgo-instrument-cold-function-only", cl::init(false), cl::Hidden,
+ cl::desc("Enable cold function only instrumentation."));
+
extern cl::opt<unsigned> MaxNumVTableAnnotations;
namespace llvm {
@@ -1897,6 +1911,11 @@ static bool skipPGOGen(const Function &F) {
return true;
if (F.getInstructionCount() < PGOFunctionSizeThreshold)
return true;
+ if (PGOInstrumentColdFunctionOnly) {
+ if (auto EntryCount = F.getEntryCount())
+ return EntryCount->getCount() > PGOColdInstrumentEntryThreshold;
+ return !PGOTreatUnknownAsCold;
+ }
return false;
}
diff --git a/llvm/test/Transforms/PGOProfile/instr-gen-cold-function.ll b/llvm/test/Transforms/PGOProfile/instr-gen-cold-function.ll
new file mode 100644
index 00000000000000..ab8cf8c010812b
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/instr-gen-cold-function.ll
@@ -0,0 +1,35 @@
+; RUN: opt < %s --passes=pgo-instr-gen -pgo-instrument-cold-function-only -pgo-function-entry-coverage -S | FileCheck --check-prefixes=COLD %s
+; RUN: opt < %s --passes=pgo-instr-gen -pgo-instrument-cold-function-only -pgo-function-entry-coverage -pgo-cold-instrument-entry-threshold=1 -S | FileCheck --check-prefixes=ENTRY-COUNT %s
+; RUN: opt < %s --passes=pgo-instr-gen -pgo-instrument-cold-function-only -pgo-function-entry-coverage -pgo-treat-unknown-as-cold -S | FileCheck --check-prefixes=UNKNOWN-FUNC %s
+
+; COLD: call void @llvm.instrprof.cover(ptr @__profn_foo, i64 [[#]], i32 1, i32 0)
+; COLD-NOT: __profn_main
+; COLD-NOT: __profn_bar
+
+; ENTRY-COUNT: call void @llvm.instrprof.cover(ptr @__profn_foo, i64 [[#]], i32 1, i32 0)
+; ENTRY-COUNT: call void @llvm.instrprof.cover(ptr @__profn_main, i64 [[#]], i32 1, i32 0)
+
+; UNKNOWN-FUNC: call void @llvm.instrprof.cover(ptr @__profn_bar, i64 [[#]], i32 1, i32 0)
+; UNKNOWN-FUNC: call void @llvm.instrprof.cover(ptr @__profn_foo, i64 [[#]], i32 1, i32 0)
+
+
+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"
+
+define void @bar() {
+entry:
+ ret void
+}
+
+define void @foo() !prof !0 {
+entry:
+ ret void
+}
+
+define i32 @main() !prof !1 {
+entry:
+ ret i32 0
+}
+
+!0 = !{!"function_entry_count", i64 0}
+!1 = !{!"function_entry_count", i64 1}
>From d9828e83d760158461baac2b3fd204465de52570 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Thu, 31 Oct 2024 09:59:15 -0700
Subject: [PATCH 2/3] Reapply "specify clang --target to fix breakage on AIX
(#114127)"
This reverts commit 06e28ed84f3f0a05b493699965e18e7a5aeaccd2.
---
clang/test/CodeGen/pgo-cold-function-coverage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/CodeGen/pgo-cold-function-coverage.c b/clang/test/CodeGen/pgo-cold-function-coverage.c
index fd1e1e7e14cda5..3003cdc3e15e02 100644
--- a/clang/test/CodeGen/pgo-cold-function-coverage.c
+++ b/clang/test/CodeGen/pgo-cold-function-coverage.c
@@ -1,7 +1,7 @@
// Test -fprofile-generate-cold-function-coverage
// RUN: rm -rf %t && split-file %s %t
-// RUN: %clang -O2 -fprofile-generate-cold-function-coverage=/xxx/yyy/ -fprofile-sample-accurate -fprofile-sample-use=%t/pgo-cold-func.prof -S -emit-llvm -o - %t/pgo-cold-func.c | FileCheck %s
+// RUN: %clang --target=x86_64 -O2 -fprofile-generate-cold-function-coverage=/xxx/yyy/ -fprofile-sample-accurate -fprofile-sample-use=%t/pgo-cold-func.prof -S -emit-llvm -o - %t/pgo-cold-func.c | FileCheck %s
// CHECK: @__llvm_profile_filename = {{.*}} c"/xxx/yyy/default_%m.profraw\00"
>From 248273230702f9c16f6334b2143334787db6f862 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Wed, 30 Oct 2024 22:21:24 -0700
Subject: [PATCH 3/3] fix datarace
---
clang/lib/Driver/ToolChains/Clang.cpp | 2 ++
.../fprofile-generate-cold-function-coverage.c | 1 +
llvm/lib/Passes/PassBuilderPipelines.cpp | 15 ++++++++++-----
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 4c6f508f1f24a6..04ae99d417d4f7 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -649,6 +649,8 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
CmdArgs.push_back(Args.MakeArgString(
Twine("--instrument-cold-function-only-path=") + Path));
CmdArgs.push_back("-mllvm");
+ CmdArgs.push_back("--pgo-instrument-cold-function-only");
+ CmdArgs.push_back("-mllvm");
CmdArgs.push_back("--pgo-function-entry-coverage");
}
diff --git a/clang/test/Driver/fprofile-generate-cold-function-coverage.c b/clang/test/Driver/fprofile-generate-cold-function-coverage.c
index 9b2f46423f34b1..a8ca69cf224cd0 100644
--- a/clang/test/Driver/fprofile-generate-cold-function-coverage.c
+++ b/clang/test/Driver/fprofile-generate-cold-function-coverage.c
@@ -1,6 +1,7 @@
// RUN: %clang -### -c -fprofile-generate-cold-function-coverage %s 2>&1 | FileCheck %s
// CHECK: "--instrument-cold-function-only-path=default_%m.profraw"
// CHECK: "--pgo-function-entry-coverage"
+// CHECK: "--pgo-instrument-cold-function-only"
// CHECK-NOT: "-fprofile-instrument"
// CHECK-NOT: "-fprofile-instrument-path=
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index c391853c8d0c2b..d5ef7c654c35cb 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -298,7 +298,9 @@ static cl::opt<bool> UseLoopVersioningLICM(
static cl::opt<std::string> InstrumentColdFuncOnlyPath(
"instrument-cold-function-only-path", cl::init(""),
- cl::desc("File path for cold function only instrumentation"), cl::Hidden);
+ cl::desc("File path for cold function only instrumentation(requires use "
+ "with --pgo-instrument-cold-function-only)"),
+ cl::Hidden);
extern cl::opt<std::string> UseCtxProfile;
extern cl::opt<bool> PGOInstrumentColdFunctionOnly;
@@ -1188,10 +1190,13 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
const bool IsCtxProfUse =
!UseCtxProfile.empty() && Phase == ThinOrFullLTOPhase::ThinLTOPreLink;
- // Enable cold function coverage instrumentation if
- // InstrumentColdFuncOnlyPath is provided.
- const bool IsColdFuncOnlyInstrGen = PGOInstrumentColdFunctionOnly =
- IsPGOPreLink && !InstrumentColdFuncOnlyPath.empty();
+ assert(
+ (InstrumentColdFuncOnlyPath.empty() || PGOInstrumentColdFunctionOnly) &&
+ "--instrument-cold-function-only-path is provided but "
+ "--pgo-instrument-cold-function-only is not enabled");
+ const bool IsColdFuncOnlyInstrGen = PGOInstrumentColdFunctionOnly &&
+ IsPGOPreLink &&
+ !InstrumentColdFuncOnlyPath.empty();
if (IsPGOInstrGen || IsPGOInstrUse || IsMemprofUse || IsCtxProfGen ||
IsCtxProfUse || IsColdFuncOnlyInstrGen)
More information about the cfe-commits
mailing list