Page MenuHomePhabricator

[llvm-profdata] Use SubCommands to simplify code
Needs ReviewPublic

Authored by ellis on Jan 18 2022, 1:03 PM.

Details

Summary

Use cl::SubCommand to improve the code in llvm-profdata.cpp in a few ways.

  • Removes arg parsing code to pick which subcommand to run
  • Removes custom help message code
  • Emits a more standard help message
  • Removes redundant options used by multiple subcommands
  • Moves all options to one place at the top of the file

Unfortunately, there are a few top-level options that llvm-profdata and llvm-profgen use. That is why MergeSubcommand is defined in InstrProf.cpp, so that the subcommand can add these top-level options.

Diff Detail

Event Timeline

ellis created this revision.Jan 18 2022, 1:03 PM
ellis published this revision for review.Jan 18 2022, 1:12 PM
ellis added reviewers: davidxl, wenlei, hoy, xur, weihe, kyulee.
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2022, 1:14 PM
wenlei added inline comments.Jan 19 2022, 11:55 AM
llvm/lib/ProfileData/InstrProf.cpp
59–66

The commands used by tools shouldn't live in ProfileData library.

llvm/lib/ProfileData/SampleProf.cpp
36

This creates a dependency from profile data to llvm-profdata, and ideally should be avoided.

Same for other changes in ProfileData

ellis added inline comments.Jan 24 2022, 3:08 PM
llvm/lib/ProfileData/SampleProf.cpp
36

Yeah, I kind of agree. I think there is a lot of value in this patch, but these options caused a lot of trouble. This patch was the cleanest solution I could come up with.

IMO there already is a dependency between SampleProf and llvm-profdata. The llvm-profdata tool calls into SampleProf and reads a global option (profile-symbol-list-cutoff). When I switched to using SubCommands, the global option was no longer recognized so I had to add the subcommands here. It might be better to add these as arguments to SampleProfileReader::create(), but that might require redundant tool options since multiple tool use this. Also this would require significant refactoring since there are several options like this.

This patch isn't necessary, but it does make the llvm-profdata tool easier to maintain and use and I'd like to find some way to do this.