[nfc][llvm-profdata] Use cl::Subcommand to organize subcommand and options in llvm-profdata #71328
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-pgo Author: Mingming Liu (minglotus-6) Changes
Full diff: https://github.com/llvm/llvm-project/pull/71328.diff 1 Files Affected:
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 7d665a8005b0d62..4d0e1549f7adc80 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -2372,9 +2372,10 @@ struct ValueSitesStats {
};
} // namespace
-static void traverseAllValueSites(const InstrProfRecord &Func, uint32_t VK,
- ValueSitesStats &Stats, raw_fd_ostream &OS,
- InstrProfSymtab *Symtab) {
+static void traverseAndShowAllValueSites(const InstrProfRecord &Func,
+ uint32_t VK, ValueSitesStats &Stats,
+ raw_fd_ostream &OS,
+ InstrProfSymtab *Symtab) {
uint32_t NS = Func.getNumValueSites(VK);
Stats.TotalNumValueSites += NS;
for (size_t I = 0; I < NS; ++I) {
@@ -2406,6 +2407,14 @@ static void traverseAllValueSites(const InstrProfRecord &Func, uint32_t VK,
}
}
+namespace {
+struct InstrProfilePerFuncOptions {
+ bool ShowCounts;
+ bool ShowIndirectCallTargets;
+ bool ShowMemOPSizes;
+};
+}; // namespace
+
static void showValueSitesStats(raw_fd_ostream &OS, uint32_t VK,
ValueSitesStats &Stats) {
OS << " Total number of sites: " << Stats.TotalNumValueSites << "\n";
@@ -2420,14 +2429,97 @@ static void showValueSitesStats(raw_fd_ostream &OS, uint32_t VK,
}
}
-static int showInstrProfile(
- const std::string &Filename, bool ShowCounts, uint32_t TopN,
- bool ShowIndirectCallTargets, bool ShowMemOPSizes, bool ShowDetailedSummary,
- std::vector<uint32_t> DetailedSummaryCutoffs, bool ShowAllFunctions,
- bool ShowCS, uint64_t ValueCutoff, bool OnlyListBelow,
- const std::string &ShowFunction, bool TextFormat, bool ShowBinaryIds,
- bool ShowCovered, bool ShowProfileVersion, bool ShowTemporalProfTraces,
- ShowFormat SFormat, raw_fd_ostream &OS) {
+static void
+showFuncPseudoCounters(const NamedInstrProfRecord &FuncRecord,
+ const InstrProfRecord::CountPseudoKind PseudoKind,
+ size_t &ShownFunctions, raw_fd_ostream &OS) {
+ if (!ShownFunctions)
+ OS << "Counters:\n";
+ ++ShownFunctions;
+ OS << " " << FuncRecord.Name << ":\n"
+ << " Hash: " << format("0x%016" PRIx64, FuncRecord.Hash) << "\n"
+ << " Counters: " << FuncRecord.Counts.size();
+ if (PseudoKind == InstrProfRecord::PseudoHot)
+ OS << " <PseudoHot>\n";
+ else if (PseudoKind == InstrProfRecord::PseudoWarm)
+ OS << " <PseudoWarm>\n";
+ else
+ llvm_unreachable("Unknown PseudoKind");
+}
+
+static void showFuncInstrProfile(const NamedInstrProfRecord &Func,
+ const bool IsIRInstr,
+ const InstrProfilePerFuncOptions &Options,
+ InstrProfSymtab *Symtab,
+ std::vector<ValueSitesStats> &VPStats,
+ size_t &ShownFunctions, raw_fd_ostream &OS) {
+ if (!ShownFunctions)
+ OS << "Counters:\n";
+
+ ++ShownFunctions;
+
+ OS << " " << Func.Name << ":\n"
+ << " Hash: " << format("0x%016" PRIx64, Func.Hash) << "\n"
+ << " Counters: " << Func.Counts.size() << "\n";
+ if (!IsIRInstr)
+ OS << " Function count: " << Func.Counts[0] << "\n";
+
+ if (Options.ShowIndirectCallTargets)
+ OS << " Indirect Call Site Count: "
+ << Func.getNumValueSites(IPVK_IndirectCallTarget) << "\n";
+
+ uint32_t NumMemOPCalls = Func.getNumValueSites(IPVK_MemOPSize);
+ if (Options.ShowMemOPSizes && NumMemOPCalls > 0)
+ OS << " Number of Memory Intrinsics Calls: " << NumMemOPCalls << "\n";
+
+ if (Options.ShowCounts) {
+ OS << " Block counts: [";
+ size_t Start = (IsIRInstr ? 0 : 1);
+ for (size_t I = Start, E = Func.Counts.size(); I < E; ++I) {
+ OS << (I == Start ? "" : ", ") << Func.Counts[I];
+ }
+ OS << "]\n";
+ }
+
+ if (Options.ShowIndirectCallTargets) {
+ OS << " Indirect Target Results:\n";
+ traverseAndShowAllValueSites(Func, IPVK_IndirectCallTarget,
+ VPStats[IPVK_IndirectCallTarget], OS, Symtab);
+ }
+
+ if (Options.ShowMemOPSizes && NumMemOPCalls > 0) {
+ OS << " Memory Intrinsic Size Results:\n";
+ traverseAndShowAllValueSites(Func, IPVK_MemOPSize, VPStats[IPVK_MemOPSize],
+ OS, nullptr);
+ }
+}
+
+static void showValueProfileStats(size_t ShownFunctions,
+ bool ShowIndirectCallTargets,
+ bool ShowMemOPSizes,
+ std::vector<ValueSitesStats> &VPStats,
+ raw_fd_ostream &OS) {
+ if (ShownFunctions && ShowIndirectCallTargets) {
+ OS << "Statistics for indirect call sites profile:\n";
+ showValueSitesStats(OS, IPVK_IndirectCallTarget,
+ VPStats[IPVK_IndirectCallTarget]);
+ }
+
+ if (ShownFunctions && ShowMemOPSizes) {
+ OS << "Statistics for memory intrinsic calls sizes profile:\n";
+ showValueSitesStats(OS, IPVK_MemOPSize, VPStats[IPVK_MemOPSize]);
+ }
+}
+
+static int
+showInstrProfile(const std::string &Filename, uint32_t TopN,
+ InstrProfilePerFuncOptions Options, bool ShowDetailedSummary,
+ std::vector<uint32_t> DetailedSummaryCutoffs,
+ bool ShowAllFunctions, bool ShowCS, uint64_t ValueCutoff,
+ bool OnlyListBelow, const std::string &ShowFunction,
+ bool TextFormat, bool ShowBinaryIds, bool ShowCovered,
+ bool ShowProfileVersion, bool ShowTemporalProfTraces,
+ ShowFormat SFormat, raw_fd_ostream &OS) {
if (SFormat == ShowFormat::Json)
exitWithError("JSON output is not supported for instr profiles");
if (SFormat == ShowFormat::Yaml)
@@ -2468,21 +2560,21 @@ static int showInstrProfile(
if (TextFormat && IsIRInstr)
OS << ":ir\n";
+ const bool IsIRLevelProfile = Reader->isIRLevelProfile();
+
for (const auto &Func : *Reader) {
- if (Reader->isIRLevelProfile()) {
- bool FuncIsCS = NamedInstrProfRecord::hasCSFlagInHash(Func.Hash);
- if (FuncIsCS != ShowCS)
+ if (IsIRLevelProfile) {
+ if (NamedInstrProfRecord::hasCSFlagInHash(Func.Hash) != ShowCS)
continue;
}
bool Show = ShowAllFunctions ||
(!ShowFunction.empty() && Func.Name.contains(ShowFunction));
- bool doTextFormatDump = (Show && TextFormat);
+ const bool doTextFormatDump = (Show && TextFormat);
if (doTextFormatDump) {
- InstrProfSymtab &Symtab = Reader->getSymtab();
- InstrProfWriter::writeRecordInText(Func.Name, Func.Hash, Func, Symtab,
- OS);
+ InstrProfWriter::writeRecordInText(Func.Name, Func.Hash, Func,
+ Reader->getSymtab(), OS);
continue;
}
@@ -2500,20 +2592,9 @@ static int showInstrProfile(
auto PseudoKind = Func.getCountPseudoKind();
if (PseudoKind != InstrProfRecord::NotPseudo) {
- if (Show) {
- if (!ShownFunctions)
- OS << "Counters:\n";
- ++ShownFunctions;
- OS << " " << Func.Name << ":\n"
- << " Hash: " << format("0x%016" PRIx64, Func.Hash) << "\n"
- << " Counters: " << Func.Counts.size();
- if (PseudoKind == InstrProfRecord::PseudoHot)
- OS << " <PseudoHot>\n";
- else if (PseudoKind == InstrProfRecord::PseudoWarm)
- OS << " <PseudoWarm>\n";
- else
- llvm_unreachable("Unknown PseudoKind");
- }
+ if (Show)
+ showFuncPseudoCounters(Func, PseudoKind, ShownFunctions, OS);
+
continue;
}
@@ -2543,47 +2624,8 @@ static int showInstrProfile(
}
if (Show) {
- if (!ShownFunctions)
- OS << "Counters:\n";
-
- ++ShownFunctions;
-
- OS << " " << Func.Name << ":\n"
- << " Hash: " << format("0x%016" PRIx64, Func.Hash) << "\n"
- << " Counters: " << Func.Counts.size() << "\n";
- if (!IsIRInstr)
- OS << " Function count: " << Func.Counts[0] << "\n";
-
- if (ShowIndirectCallTargets)
- OS << " Indirect Call Site Count: "
- << Func.getNumValueSites(IPVK_IndirectCallTarget) << "\n";
-
- uint32_t NumMemOPCalls = Func.getNumValueSites(IPVK_MemOPSize);
- if (ShowMemOPSizes && NumMemOPCalls > 0)
- OS << " Number of Memory Intrinsics Calls: " << NumMemOPCalls
- << "\n";
-
- if (ShowCounts) {
- OS << " Block counts: [";
- size_t Start = (IsIRInstr ? 0 : 1);
- for (size_t I = Start, E = Func.Counts.size(); I < E; ++I) {
- OS << (I == Start ? "" : ", ") << Func.Counts[I];
- }
- OS << "]\n";
- }
-
- if (ShowIndirectCallTargets) {
- OS << " Indirect Target Results:\n";
- traverseAllValueSites(Func, IPVK_IndirectCallTarget,
- VPStats[IPVK_IndirectCallTarget], OS,
- &(Reader->getSymtab()));
- }
-
- if (ShowMemOPSizes && NumMemOPCalls > 0) {
- OS << " Memory Intrinsic Size Results:\n";
- traverseAllValueSites(Func, IPVK_MemOPSize, VPStats[IPVK_MemOPSize], OS,
- nullptr);
- }
+ showFuncInstrProfile(Func, IsIRInstr, Options, , &(Reader->getSymtab()),
+ VPStats, ShownFunctions, OS);
}
}
if (Reader->hasError())
@@ -2621,16 +2663,8 @@ static int showInstrProfile(
OS << " " << hotfunc.first << ", max count = " << hotfunc.second << "\n";
}
- if (ShownFunctions && ShowIndirectCallTargets) {
- OS << "Statistics for indirect call sites profile:\n";
- showValueSitesStats(OS, IPVK_IndirectCallTarget,
- VPStats[IPVK_IndirectCallTarget]);
- }
-
- if (ShownFunctions && ShowMemOPSizes) {
- OS << "Statistics for memory intrinsic calls sizes profile:\n";
- showValueSitesStats(OS, IPVK_MemOPSize, VPStats[IPVK_MemOPSize]);
- }
+ showValueProfileStats(ShownFunctions, ShowIndirectCallTargets, ShowMemOPSizes,
+ VPStats, OS);
if (ShowDetailedSummary) {
OS << "Total number of blocks: " << PS->getNumCounts() << "\n";
@@ -3044,11 +3078,14 @@ static int show_main(int argc, const char *argv[]) {
if (ProfileKind == instr)
return showInstrProfile(
- Filename, ShowCounts, TopNFunctions, ShowIndirectCallTargets,
- ShowMemOPSizes, ShowDetailedSummary, DetailedSummaryCutoffs,
- ShowAllFunctions, ShowCS, ValueCutoff, OnlyListBelow, ShowFunction,
- TextFormat, ShowBinaryIds, ShowCovered, ShowProfileVersion,
- ShowTemporalProfTraces, SFormat, OS);
+ Filename, TopNFunctions,
+ InstrProfilePerFuncOptions{.ShowCounts = ShowCounts,
+ .ShowIndirectCallTargets =
+ ShowIndirectCallTargets,
+ .ShowMemOPSizes = ShowMemOPSizes},
+ ShowDetailedSummary, DetailedSummaryCutoffs, ShowAllFunctions, ShowCS,
+ ValueCutoff, OnlyListBelow, ShowFunction, TextFormat, ShowBinaryIds,
+ ShowCovered, ShowProfileVersion, ShowTemporalProfTraces, SFormat, OS);
if (ProfileKind == sample)
return showSampleProfile(Filename, ShowCounts, TopNFunctions,
ShowAllFunctions, ShowDetailedSummary,
|
| @@ -2406,6 +2407,14 @@ static void traverseAllValueSites(const InstrProfRecord &Func, uint32_t VK, | |||
| } | |||
| } | |||
|
|
|||
| namespace { | |||
| struct InstrProfilePerFuncOptions { | |||
| bool ShowCounts; | |||
There was a problem hiding this comment.
please initialize scalars at declaration (i.e. bool ShowCounts = false), it's very easy to later write code that forgets to initialize a field, and while maybe noticing compiler warnings can highlight this, it's so much easier to just initialize at declaration and avoid undefined behavior!
| size_t &ShownFunctions, raw_fd_ostream &OS) { | ||
| if (!ShownFunctions) | ||
| OS << "Counters:\n"; | ||
| ++ShownFunctions; |
There was a problem hiding this comment.
It's currently an input and output parameter. If I understand correctly, "returning it" means the following code sequence
in caller of `showFuncPseudoCounters`
ShownFunctions = showFuncPseudoCounters(ShownFunctions)
`showFuncPseudoCounters` takes 'ShownFunctions' as const input parameter and returns its 'increment-by-one' value
Is this generally more preferred in llvm codebase than using an input & output parameter?
| static void showValueProfileStats(size_t ShownFunctions, | ||
| bool ShowIndirectCallTargets, | ||
| bool ShowMemOPSizes, | ||
| std::vector<ValueSitesStats> &VPStats, |
There was a problem hiding this comment.
is VPStats not const-ed because of the use of operator[]? If so, is the desired behavior to insert a default value if IPVK_MemOpSize isn't a key?
There was a problem hiding this comment.
The vector 'VPStats' is initialized as std::vector<ValueSitesStats> VPStats(NumVPKind); code, and out-of-bound access from operator [] is undefined behavior.
I updated this new function and its callee to take a constant vector.
| } | ||
|
|
||
| static int | ||
| showInstrProfile(const std::string &Filename, uint32_t TopN, |
There was a problem hiding this comment.
I wonder if factoring all the show... APIs into a class would make more sense - do some of the parameters make sense as shared state between these functions?
There was a problem hiding this comment.
I wonder if factoring all the show... APIs into a class would make more sense - do some of the parameters make sense as shared state between these functions?
To share some context of this NFC, the immediate (and motivating) use case InstrProfilePerFuncOptions is to allow showing one new type of value profiles without increasing the length of function arguments.
InstrProfileShowOptions (a profile-level show options) would make sense as a separate patch. How to factor them is a more open-ended question. With all (nearly scalar) options, some options could be used together (e.g., TopN is meaningful to show function profiles but not meaningful in temporal profile traces) while other options are more independent of each other.
| @@ -2406,6 +2407,14 @@ static void traverseAllValueSites(const InstrProfRecord &Func, uint32_t VK, | |||
| } | |||
| } | |||
|
|
|||
| namespace { | |||
| struct InstrProfilePerFuncOptions { | |||
There was a problem hiding this comment.
Will it be limited to InstrProfile? Perhaps just name it "PerFuncOptions"?
There was a problem hiding this comment.
Showing indirect call targets seems implicit currently; showSampleProfile doesn't take an option to suppress indirect call targets.
A minor thing is, functions in sample profiles doesn't have mem-op size; so passing 'PerFuncOptions' in showSampleProfile static function would require an option invalidation
if (PerFuncOptions.ShowMemOPSizes)
emit error with message "mem-op size is not supported in sample profiles"
The struct is in anonymous namespace which makes it easier for renames if necessary.
There was a problem hiding this comment.
Lack of mem-op size is just a missing feature. Adding a warning and ignore would be fine.
|
The patch is currently adding new C++ structs to reduce the number of arguments. Another option is to move option definitions ( llvm-project/llvm/tools/llvm-profdata/llvm-profdata.cpp Lines 2954 to 3042 in 45ca24e namespace show_prof{), so the option definitions are visible to showInstrProfile and showSampleProfile (no need to pass long list of parameters) . The downside is that, not all options apply to both functions.
Thoughts about these two options? |
Moving show options to show_prof namespace looks reasonable to me. Need to make sure the --help option for the 'show' command works properly though. |
minglotus-6
force-pushed
the
llvm-profdata
branch
from
f4261ef
to
e9b590a
Compare
minglotus-6
changed the title
|
I updated the PR to exemplify the change for |
| @@ -2406,6 +2407,14 @@ static void traverseAllValueSites(const InstrProfRecord &Func, uint32_t VK, | |||
| } | |||
| } | |||
|
|
|||
| namespace { | |||
| struct InstrProfilePerFuncOptions { | |||
There was a problem hiding this comment.
Lack of mem-op size is just a missing feature. Adding a warning and ignore would be fine.
Sounds good. (Learnt from failed error message) I should probably register llvm-project/llvm/tools/llvm-pdbutil/llvm-pdbutil.cpp Lines 163 to 164 in c2ad9f8 Without that, I'm getting runtime registration errors |
namespace.
- The change uses cl::SubCommand feature that allow to register options
under a subcommand.
- Without using cl::SubCommand, the options in the show name namespace will
take a registered name as global variables, causing runtime errors when a
function-scope static option tries to add a function of a same name.
- Make changes in the CommandLine library accordingly.
minglotus-6
changed the title
| static int showInstrProfile(const std::string &Filename, ShowFormat SFormat, | ||
| raw_fd_ostream &OS) { | ||
| static int showInstrProfile( | ||
| const std::string &Filename, bool ShowCounts, uint32_t TopN, |
There was a problem hiding this comment.
Why still passing the options as parameters ? They can be referenced as show_options:: in the function body.
There was a problem hiding this comment.
I updated {showInstrProfile, showSampleProfile, showMemProfProfile, showDebugInfoCorrelation} to use options directly.
Different subcommand could have |
With the usage of
I might need to take this back. It seems plausible to associate one |
…ptions for a custom subcommand. (#71776) **Context:** - In https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html and commit 07670b3, `cl::SubCommand` is introduced. - Options that don't specify subcommand goes into a special 'top level' subcommand. **Motivating Use Case:** - The motivating use case is to refactor `llvm-profdata` to use `cl::SubCommand` to organize subcommands. See #71328. A valid use case that's not supported before this patch is shown below ``` // show-option{1,2} are associated with 'show' subcommand. // top-level-option3 is in top-level subcomand (e.g., `profile-isfs` in SampleProfReader.cpp) llvm-profdata show --show-option1 --show-option2 --top-level-option3 ``` - Before this patch, option handler look-up will fail with the following error message "Unknown command line argument --top-level-option3". - After this patch, option handler look-up will look up in sub-command options first, and use top-level subcommand as a fallback, so 'top-level-option3' is parsed correctly.
|
This is ready for review again! Some options are sub-command specific and I tried to keep these options closer to where they are used. Common options are moved ahead at the very beginning. |
minglotus-6
changed the title
|
To ensure options are not deleted mistakenly, I run
[1] |
…ooking options for a custom subcommand (#71981) Fixed build bot errors. - Use `StackOption<std::string>` type for the top level option. This way, a per test-case option is unregistered when destructor of `StackOption` cleans up state for subsequent test cases. - Repro the crash with no test sharding `/usr/bin/python3 /path/to/llvm-project/build/./bin/llvm-lit -vv --no-gtest-sharding -j128 /path/to/llvm-project/llvm/test/Unit`. The crash is gone with the fix (same no-sharding repro) **Original commit message:** **Context:** - In https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html and commit 07670b3, `cl::SubCommand` is introduced. - Options that don't specify subcommand goes into a special 'top level' subcommand. **Motivating Use Case:** - The motivating use case is to refactor `llvm-profdata` to use `cl::SubCommand` to organize subcommands. See #71328. A valid use case that's not supported before this patch is shown below ``` // show-option{1,2} are associated with 'show' subcommand. // top-level-option3 is in top-level subcomand (e.g., `profile-isfs` in SampleProfReader.cpp) llvm-profdata show --show-option1 --show-option2 --top-level-option3 ``` - Before this patch, option handler look-up will fail with the following error message "Unknown command line argument --top-level-option3". - After this patch, option handler look-up will look up in sub-command options first, and use top-level subcommand as a fallback, so 'top-level-option3' is parsed correctly.
| cl::SubCommand | ||
| OverlapSubcommand("overlap", | ||
| "Computes and displays the overlap between two profiles"); | ||
| cl::SubCommand MergeSubcommand("merge", "Merges profiles"); |
There was a problem hiding this comment.
A link to the relevant documentation would be a helpful to users who don't look at the source: For example, for merge you can add a link to https://llvm.org/docs/CommandGuide/llvm-profdata.html#profdata-merge
Same for the others. Also drop the comment on L50 if you choose to adopt this suggestion.
| uint64_t MaxTraceLength, int MaxDbgCorrelationWarnings, | ||
| bool OutputSparse, unsigned NumThreads, FailureMode FailMode, | ||
| const StringRef ProfiledBinary) { | ||
| // Common options. |
There was a problem hiding this comment.
Perhaps we can move the common and 2-common set of options to the top of the file, right under subcommands. Since the options are documented (i.e. "options unique to show" etc) I also wouldn't mind moving everything to the top of the file. I don't see a particular reason to keep them near the methods which implement the functionality, particularly since we always parse all options now. What do you think?
There was a problem hiding this comment.
Sounds reasonable to me. I moved options definitions right after cl::Subcommand.
| cl::desc("For context sensitive PGO counts. Does not work with CSSPGO."), | ||
| cl::sub(OverlapSubcommand)); | ||
|
|
||
| cl::opt<std::string> FuncNameFilter( |
There was a problem hiding this comment.
Should this be in 2-common options since it's used by show and overlap?
There was a problem hiding this comment.
good catch. I moved it to the 2-common options section.
| cl::SubCommand | ||
| OverlapSubcommand("overlap", | ||
| "Computes and displays the overlap between two profiles"); | ||
| cl::SubCommand MergeSubcommand("merge", "Merges profiles"); |
| uint64_t MaxTraceLength, int MaxDbgCorrelationWarnings, | ||
| bool OutputSparse, unsigned NumThreads, FailureMode FailMode, | ||
| const StringRef ProfiledBinary) { | ||
| // Common options. |
There was a problem hiding this comment.
Sounds reasonable to me. I moved options definitions right after cl::Subcommand.
| cl::desc("For context sensitive PGO counts. Does not work with CSSPGO."), | ||
| cl::sub(OverlapSubcommand)); | ||
|
|
||
| cl::opt<std::string> FuncNameFilter( |
There was a problem hiding this comment.
good catch. I moved it to the 2-common options section.
…ptions for a custom subcommand. (llvm#71776) **Context:** - In https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html and commit 07670b3, `cl::SubCommand` is introduced. - Options that don't specify subcommand goes into a special 'top level' subcommand. **Motivating Use Case:** - The motivating use case is to refactor `llvm-profdata` to use `cl::SubCommand` to organize subcommands. See llvm#71328. A valid use case that's not supported before this patch is shown below ``` // show-option{1,2} are associated with 'show' subcommand. // top-level-option3 is in top-level subcomand (e.g., `profile-isfs` in SampleProfReader.cpp) llvm-profdata show --show-option1 --show-option2 --top-level-option3 ``` - Before this patch, option handler look-up will fail with the following error message "Unknown command line argument --top-level-option3". - After this patch, option handler look-up will look up in sub-command options first, and use top-level subcommand as a fallback, so 'top-level-option3' is parsed correctly.
…ooking options for a custom subcommand (llvm#71981) Fixed build bot errors. - Use `StackOption<std::string>` type for the top level option. This way, a per test-case option is unregistered when destructor of `StackOption` cleans up state for subsequent test cases. - Repro the crash with no test sharding `/usr/bin/python3 /path/to/llvm-project/build/./bin/llvm-lit -vv --no-gtest-sharding -j128 /path/to/llvm-project/llvm/test/Unit`. The crash is gone with the fix (same no-sharding repro) **Original commit message:** **Context:** - In https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html and commit llvm@07670b3, `cl::SubCommand` is introduced. - Options that don't specify subcommand goes into a special 'top level' subcommand. **Motivating Use Case:** - The motivating use case is to refactor `llvm-profdata` to use `cl::SubCommand` to organize subcommands. See llvm#71328. A valid use case that's not supported before this patch is shown below ``` // show-option{1,2} are associated with 'show' subcommand. // top-level-option3 is in top-level subcomand (e.g., `profile-isfs` in SampleProfReader.cpp) llvm-profdata show --show-option1 --show-option2 --top-level-option3 ``` - Before this patch, option handler look-up will fail with the following error message "Unknown command line argument --top-level-option3". - After this patch, option handler look-up will look up in sub-command options first, and use top-level subcommand as a fallback, so 'top-level-option3' is parsed correctly.
…tions in llvm-profdata (llvm#71328) - The motivation is to reduce the number of arguments passed around (e.g., from `show_main` to `show*Profile`). In order to do this, move function-defined options to global variables, and create `cl::SubCommand` for {show, merge, overlap, order} to organize options. - The side-effect by extracting function local options to a C++ namespace is that the extracted options are no longer (lazily) initialized when the enclosing function runs for the first time. - `cl::Subcommand` support (introduced in https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html) could put options in a per-subcommand namespace. - One option could belong to multiple subcommand. This patch defines most of the options once and associates them with multiple subcommands except 1. `overlap` and `show` both has `value-cutoff` with different default values ([former](https://github.com/llvm/llvm-project/blob/64f62de96609dc3ea9a8a914a9e9445b7f4d625d/llvm/tools/llvm-profdata/llvm-profdata.cpp#L2352) vs [latter](https://github.com/llvm/llvm-project/blob/64f62de96609dc3ea9a8a914a9e9445b7f4d625d/llvm/tools/llvm-profdata/llvm-profdata.cpp#L3009)). Define 'OverlapValueCutoff' and 'ShowValueCutoff' respectively. 2. `show` supports three profile formats in `ProfileKind` while {`merge`, `overlap`} supports two. Define separate options. - Clean up obsolete code as a result, including `-h` and `--version` customizations. These two options are supported for all commands. Results pasted. - [-h and --help](https://gist.github.com/minglotus-6/387490e5eeda2dd2f9c440a424d6f360) output. - [--version](https://gist.github.com/minglotus-6/f905abcc3a346957bd797f2f84c18c1b) - [llvm-profdata show --help](https://gist.github.com/minglotus-6/f143079f02af243a94758138c0af471a) This PR should be `llvm-profdata` only. It depends on llvm#71981
#71328 refactored `llvm-profdata.cpp` to use subcommands (which is super nice), but left many unused `argv` variables. This opts to use `ProgName` where necessary, and removes `argv` otherwise.
show_maintoshow*Profile). In order to do this, move function-defined options to global variables, and createcl::SubCommandfor {show, merge, overlap, order} to organize options.cl::Subcommandsupport (introduced in https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html) could put options in a per-subcommand namespace.overlapandshowboth hasvalue-cutoffwith different default values (former vs latter). Define 'OverlapValueCutoff' and 'ShowValueCutoff' respectively.showsupports three profile formats inProfileKindwhile {merge,overlap} supports two. Define separate options.-hand--versioncustomizations. These two options are supported for all commands. Results pasted.This PR should be
llvm-profdataonly. It depends on #71981