-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[BOLT][merge-fdata]Fix support for fdata files starting with no_lbr on ARM platform #112328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason to change the formatting? Did you change any options? |
||||||
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; | ||||||
Comment on lines
256
to
+257
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we avoid the use of mutexes here by just setting bolted_collection/no_lbr per profile and checking that after we parse all profiles? |
||||||
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"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
for (const auto &[Key, Value] : MergedProfile) | ||||||
output() << Key << " " << Value << "\n"; | ||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep formatting as is here