[BOLT][merge-fdata]Fix support for fdata files starting with no_lbr on ARM platform #112328
Conversation
…, which was not user-friendly for the ARM platform. Therefore, I fixed this issue to enable its use on the ARM platform (with no_lbr). Additionally, I validated the merging of samples across both LBR and Non-LBR scenarios.
zrxxzz
requested review from
aaupov,
maksfb,
rafaelauler,
ayermolo and
dcci
as code owners
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-bolt Author: zrx_p4ul (zrxxzz) ChangesThe merge-fdata tool did not support fdata files starting with no_lbr, which was not user-friendly for the ARM platform. Full diff: https://github.com/llvm/llvm-project/pull/112328.diff 3 Files Affected:
diff --git a/bolt/test/merge-fdata-between-no-lbr-and-lbr.test b/bolt/test/merge-fdata-between-no-lbr-and-lbr.test
new file mode 100644
index 00000000000000..1a7efccd45d817
--- /dev/null
+++ b/bolt/test/merge-fdata-between-no-lbr-and-lbr.test
@@ -0,0 +1,14 @@
+## Test that merge-fdata fails to mix fdata files with and without the no_lbr marker.
+
+# REQUIRES: system-linux
+
+# RUN: split-file %s %t
+# RUN: not merge-fdata %t/a.fdata %t/b.fdata 2>&1 | FileCheck %s
+
+# CHECK: cannot mix profile collected on LBR and non-LBR architectures
+
+#--- a.fdata
+no_lbr
+main 1
+#--- b.fdata
+main 1
diff --git a/bolt/test/merge-fdata-between-two-no-lbr.test b/bolt/test/merge-fdata-between-two-no-lbr.test
new file mode 100644
index 00000000000000..e2a3df7716afd5
--- /dev/null
+++ b/bolt/test/merge-fdata-between-two-no-lbr.test
@@ -0,0 +1,17 @@
+## Test that merge-fdata correctly handles merging two fdata files with the no_lbr marker.
+
+# REQUIRES: system-linux
+
+# RUN: split-file %s %t
+# RUN: merge-fdata %t/a.fdata %t/b.fdata -o %t/merged.fdata
+# RUN: FileCheck %s --input-file %t/merged.fdata
+
+# CHECK: no_lbr
+# CHECK: main 2
+
+#--- a.fdata
+no_lbr
+main 1
+#--- b.fdata
+no_lbr
+main 1
diff --git a/bolt/tools/merge-fdata/merge-fdata.cpp b/bolt/tools/merge-fdata/merge-fdata.cpp
index 89ca46c1c0a8fa..ef85fd345d4505 100644
--- a/bolt/tools/merge-fdata/merge-fdata.cpp
+++ b/bolt/tools/merge-fdata/merge-fdata.cpp
@@ -34,45 +34,32 @@ cl::OptionCategory MergeFdataCategory("merge-fdata options");
enum SortType : char {
ST_NONE,
- ST_EXEC_COUNT, /// Sort based on function execution count.
- ST_TOTAL_BRANCHES, /// Sort based on all branches in the function.
+ ST_EXEC_COUNT, /// Sort based on function execution count.
+ ST_TOTAL_BRANCHES, /// Sort based on all branches in the function.
};
static cl::list<std::string>
-InputDataFilenames(
- cl::Positional,
- cl::CommaSeparated,
- cl::desc("<fdata1> [<fdata2>]..."),
- cl::OneOrMore,
- cl::cat(MergeFdataCategory));
-
-static cl::opt<SortType>
-PrintFunctionList("print",
- cl::desc("print the list of objects with count to stderr"),
- cl::init(ST_NONE),
- cl::values(clEnumValN(ST_NONE,
- "none",
- "do not print objects/functions"),
- clEnumValN(ST_EXEC_COUNT,
- "exec",
- "print functions sorted by execution count"),
- clEnumValN(ST_TOTAL_BRANCHES,
- "branches",
- "print functions sorted by total branch count")),
- cl::cat(MergeFdataCategory));
-
-static cl::opt<bool>
-SuppressMergedDataOutput("q",
- cl::desc("do not print merged data to stdout"),
- cl::init(false),
- cl::Optional,
- cl::cat(MergeFdataCategory));
-
-static cl::opt<std::string>
-OutputFilePath("o",
- cl::value_desc("file"),
- cl::desc("Write output to <file>"),
- cl::cat(MergeFdataCategory));
+ InputDataFilenames(cl::Positional, cl::CommaSeparated,
+ cl::desc("<fdata1> [<fdata2>]..."), cl::OneOrMore,
+ cl::cat(MergeFdataCategory));
+
+static cl::opt<SortType> PrintFunctionList(
+ "print", cl::desc("print the list of objects with count to stderr"),
+ cl::init(ST_NONE),
+ cl::values(clEnumValN(ST_NONE, "none", "do not print objects/functions"),
+ clEnumValN(ST_EXEC_COUNT, "exec",
+ "print functions sorted by execution count"),
+ clEnumValN(ST_TOTAL_BRANCHES, "branches",
+ "print functions sorted by total branch count")),
+ cl::cat(MergeFdataCategory));
+
+static cl::opt<bool> SuppressMergedDataOutput(
+ "q", cl::desc("do not print merged data to stdout"), cl::init(false),
+ cl::Optional, cl::cat(MergeFdataCategory));
+
+static cl::opt<std::string> OutputFilePath("o", cl::value_desc("file"),
+ cl::desc("Write output to <file>"),
+ cl::cat(MergeFdataCategory));
} // namespace opts
@@ -265,7 +252,10 @@ bool isYAML(const StringRef Filename) {
void mergeLegacyProfiles(const SmallVectorImpl<std::string> &Filenames) {
errs() << "Using legacy profile format.\n";
std::optional<bool> BoltedCollection;
+ std::optional<bool> NoLbr;
std::mutex BoltedCollectionMutex;
+ std::mutex NoLbrMutex;
+ std::string NoLbrLabel;
typedef StringMap<uint64_t> ProfileTy;
auto ParseProfile = [&](const std::string &Filename, auto &Profiles) {
@@ -298,6 +288,25 @@ void mergeLegacyProfiles(const SmallVectorImpl<std::string> &Filenames) {
BoltedCollection = false;
}
+ std::lock_guard<std::mutex> Lock1(NoLbrMutex);
+ // Check if the string "no_lbr" is in the first line
+ if (Buf.starts_with("no_lbr")) {
+ if (!NoLbr.value_or(true))
+ report_error(
+ Filename,
+ "cannot mix profile collected on LBR and non-LBR architectures");
+ NoLbr = true;
+ size_t Pos = Buf.find("\n");
+ NoLbrLabel = Buf.substr(0, Pos).str();
+ Buf = Buf.drop_front(Pos + 1);
+ } else {
+ if (NoLbr.value_or(false))
+ report_error(
+ Filename,
+ "cannot mix profile collected on LBR and non-LBR architectures");
+ NoLbr = false;
+ }
+
Profile = &Profiles[tid];
}
@@ -336,6 +345,8 @@ void mergeLegacyProfiles(const SmallVectorImpl<std::string> &Filenames) {
if (BoltedCollection.value_or(false))
output() << "boltedcollection\n";
+ if (NoLbr.value_or(false))
+ output() << NoLbrLabel << "\n";
for (const auto &[Key, Value] : MergedProfile)
output() << Key << " " << Value << "\n";
|
|
Hi @yota9 I hope this message finds you well. I have submitted a pull request that addresses an issue with the Could you please review my PR when you have a moment? Your feedback would be greatly appreciated. Thank you for your time and consideration. Best regards |
|
Through testing, it has been proven that even without LBR, merging samples on the ARM platform still achieves good results for MySQL. |
|
Hello, @maksfb could you please take a look at this pull request? Thank you. |
The merge-fdata tool did not support fdata files starting with no_lbr, which was not user-friendly for the ARM platform.
Therefore, I fixed this issue to enable its use on the ARM platform (with no_lbr). Additionally, I validated the merging of samples across both LBR and Non-LBR scenarios.