Page MenuHomePhabricator

[CommandLine] WIP: Add new debug_opt that reduces down to size of DataType in Release builds.
Needs ReviewPublic

Authored by hintonda on May 22 2019, 11:13 AM.

Details

Reviewers
beanz
jfb
Summary

Added a new Option template, cl::debug_opt, to the
CommandLine parser that aliases to cl::opt in debug builds, but
decays to a simple class containing only the initialized DataType in
Release builds.

debug_opts are also removed from the parser in release builds, so
they don't take up any additional memory, and are designed to be a
drop-in replacement for cl::opt.

cl::OptionCategory and cl::SubCommand also have debug versions
which disappear in release builds.

Event Timeline

hintonda created this revision.May 22 2019, 11:13 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: hiraditya. · View Herald Transcript
hintonda marked 2 inline comments as done.May 22 2019, 11:20 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
411–412

Templatizing these methods is needed, but could go in its own patch. It seems some were and some weren't, but all are needed to use debug_opt.

llvm/lib/Support/Signals.cpp
42

This was added as an example usage in real code, but doesn't need to be included in this patch if and when lands.

hintonda updated this revision to Diff 200825.EditedMay 22 2019, 2:56 PM
  • Added cl::debug_opt_storage with specializations.

With this change, the current sizes are:

Debug build (note that this doesn't include the size of ArgStr or Desc):

sizeof cl::Option: 136
sizeof cl::opt<bool>: 168
sizeof cl::opt<bool, true> -- with location: 168
sizeof cl::debug_opt<bool>: 168
sizeof cl::debug_opt<bool, true>  with location: 168

Release build (ArgStr and Desc are dropped for cl::debug_opt versions):

sizeof cl::Option: 128
sizeof cl::opt<bool>: 160
sizeof cl::opt<bool, true> -- with location: 160
sizeof cl::debug_opt<bool>: 1
sizeof cl::debug_opt<bool, true>  with location: 8
beanz added a comment.May 22 2019, 3:06 PM

A very interesting idea. A few thoughts inline.

llvm/include/llvm/Support/CommandLine.h
2019

I suspect that NDEBUG probably isn't the right gating for this. We actually probably want this to be something controlled by a build system option.

llvm/unittests/Support/CommandLineTest.cpp
99

I'm a little concerned about how this would work. For one thing it makes testing and reproducing bugs in release builds trickier. I suspect with your patch as-is if you converted many of the target options to debug_opt check-llvm would fail gloriously. I'm not sure that is the behavior we want because I know many distribution workflows rely on running the tests against the build before packaging the distribution.

hintonda marked an inline comment as done.May 22 2019, 3:41 PM
hintonda added inline comments.
llvm/unittests/Support/CommandLineTest.cpp
99

This version gets rid of as much as possible -- sort of a benchmark.

Depending on what we want/need for release tests, I could see memory saving alternatives that would allow the tests to run as is. However, I'd imagine there are still plenty of cases where this approach would work out fine.

For example, there are already cases where Options are completely #ifdef'd away, e.g., --debug and friends. The problem with that approach compared with this one is that it requires you litter the code with #ifdef's. Assuming we ultimately get the savings my tests indicate, this approach would help keep the code clean at the cost of a very small amount of memory.