This is an archive of the discontinued LLVM Phabricator instance.

Allow suppressing host and target info in VersionPrinter
ClosedPublic

Authored by pammon on Mar 13 2017, 11:19 AM.

Details

Summary

VersionPrinter by default outputs information about the Host CPU
and Default target. Printing this information requires linking in
a large amount of data, such as supported target triples as C
strings, which in turn bloats the binary size.

Enable a new CMake option LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO
which controls printing of the host and target info. This allows
the target triple names to be dead-code stripped. This is a nice
win for LLVM clients that wish to minimize their binary size, such
as graphics drivers.

By default this is ON, so there is no change in the default behavior.
Clients who wish to suppress this printing can do so by setting this
option to off via CMake.

A test app on Linux that uses ParseCommandLineOptions() shows a binary
size reduction of 23KB (from 149K to 126K) for a Release build, and 24KB
(from 135K to 111K) in a MinSizeRel build.

Diff Detail

Repository
rL LLVM

Event Timeline

pammon created this revision.Mar 13 2017, 11:19 AM
compnerd edited reviewers, added: chandlerc, compnerd; removed: mgorny.Mar 20 2017, 4:18 PM
compnerd added a subscriber: mgorny.
chandlerc edited edge metadata.Mar 21 2017, 12:15 PM

This seems like a totally nice direction, but I wonder if I could convince you to scope creep this a bit?

I feel like this shouldn't be guarded by a macro at all. Instead, for tools where it makes sense, we should have an API they can call (perhaps instead of or in addition to this) which provides this level of information in their version strings.

For example, I don't ever want clang-format or FileCheck to care about this. But I'd rather like opt, llc, and clang to always show it. And for those, it costs them nothing as this code is going to get linked in anyways.

What do you think?

compnerd edited edge metadata.Mar 21 2017, 1:01 PM

I think that the idea that you propose is good, however, I think that may be better to do as a follow up. This would get the immediate benefit for the tooling, and we can work out a nicer way to expose the information to the other users. Making it something which each app does means that we would need to change all the apps as well, which would be a much larger change.

I think that the idea that you propose is good, however, I think that may be better to do as a follow up. This would get the immediate benefit for the tooling, and we can work out a nicer way to expose the information to the other users. Making it something which each app does means that we would need to change all the apps as well, which would be a much larger change.

OK, but if you want to go the current route, it needs more work too.

First, there is no corresponding CMake change to allow us to set the macro.

But once you do that, we'll get to the core issue I think I have here. I don't think we want to lose this information from tools like opt, llc, and clang where it is *very* useful. So that'll cause the default for this CMake variable to be set in the direction that doesn't get us any benefit by default. If that's enough for you, then OK. But if you want this to be available for tooling *by default* I think we need to at least do the initial work of making this controllable to avoid regressing the debug information provided in Clang and other binary's version string.

Are the changes in config.h.cmake not sufficient to enable setting this variable? I was able to set the variable in my tests by invoke cmake with -DLLVM_VERSION_PRINTER_NO_HOST_TARGET_INFO=1.

I'm not proposing this be set by default. This is something that clients would need to opt into, via the cmake option above.

I think that @chandlerc is suggesting making it a CMake option (see the option function in CMake) to provide documentation for this for users. I think that is a pretty reasonable ask, as it aids in discoverability of this.

pammon edited the summary of this revision. (Show Details)Mar 22 2017, 3:49 PM
pammon removed a subscriber: mgorny.
pammon updated this revision to Diff 92729.Mar 22 2017, 3:52 PM

Use CMake's option() to improve discoverability. Invert sense of option to improve clarity.

@chandlerc will you please take a look at my most recent changes? Thank you

compnerd accepted this revision.Apr 18 2017, 1:32 PM

This is opt-out by default, so it shouldn't have any impact on users by default. When building for tooling, this should allow smaller builds for those.

This revision is now accepted and ready to land.Apr 18 2017, 1:32 PM
This revision was automatically updated to reflect the committed changes.

Committed for coworker.