This is an archive of the discontinued LLVM Phabricator instance.

Add _LIBCPP_LARGE_CODEBASE
AbandonedPublic

Authored by thomasanderson on May 25 2018, 4:56 PM.

Details

Summary

This CL adds a tweak to be used by large codebases to noinline certain
commonly-used functions. This can reduce binary size considerably for some
codebases. For example, Chromium on Linux was reduced by 4.51MiB or 3.31%.

Diff Detail

Event Timeline

thomasanderson created this revision.May 25 2018, 4:56 PM
thomasanderson retitled this revision from [WIP] Add CHROMIUM_CXX_TWEAK_INLINES to Add _LIBCPP_LARGE_CODEBASE.May 30 2018, 1:01 PM
thomasanderson edited the summary of this revision. (Show Details)
thomasanderson added a reviewer: thakis.

+thakis do you know who should review this?

pcc added subscribers: llvm-commits, pcc.

Probably either mclow or EricWF.

Pinging mclow or EricWF

EricWF added inline comments.Jun 4 2018, 11:33 AM
include/__config
782

This should be documented in docs/DesignDocs/VisibilityMacros.rst instead of in the header.

812

I don't have a strong objection to the intention of the patch, but I really dislike the name.

814

I understand wanting to disable the forced inlining of functions, but I don't understand why you would want to explicitly disable inlining.
It seems like if the function shouldn't be inlined for performance reasons, that should be controlled by an optimizer flag, and if the optimizer isn't producing ideal code, that seems like an optimizer bug.

EricWF added inline comments.Jun 4 2018, 11:36 AM
include/string
802

Most of these functions *can't* be inlined by the compiler unless the inline keyword is explicitly specified because these functions are externally instantiated.

I suspect not only that these functions not need __attribute__((noinline)) but that would implicitly have that effect if __attribute__((always_inline)) was omitted.

As previously mentioned, most of these changes deal with symbols which are externally instantiated. These symbols cannot be inlined unless the inline keyword is explicitly specified. The addition of __attribute__((noinline)) likely has no effect in a large number of cases.

thomasanderson added inline comments.
include/__config
814

I'm building Chromium on Linux with lld (without LTO) with -O2. With the attribute((noinline))'s from this patch, the binary size is 194M, 203296816B. Without the noinline's, it's 197M, 206227536B.

If I use -Os instead, the measurements are 191M, 200064384B and 192M, 200578400B. Better, but still not perfect.

+pcc do you know why lld is having trouble noinlining these functions?

pcc added inline comments.Jun 4 2018, 6:09 PM
include/__config
814

Since you have LTO disabled, the choice of linker shouldn't make a difference with regards to inlining here. (Even with LTO you shouldn't see a difference with Chromium because Chromium links with --lto-O0.)

Note that we're not trying to avoid inlining for performance reasons but for code size reasons. Finding a good heuristic for the latter in the inliner is, unfortunately, an unsolved problem as of yet. (One of the reasons is that the heuristic would need be something of the form "if I inline this, will it unblock other size optimizations on the inlined code" and LLVM's optimizers just don't work that way, at least not yet.)

include/string
802

You might consider moving these attributes onto the definitions, which is where the inline keyword should appear (or not appear) anyway.

thomasanderson marked 2 inline comments as done.Jun 14 2018, 5:32 PM
thomasanderson added inline comments.
include/__config
812

Changed. I'm also open to suggestions, so if you'd like anything in particular, plmk.

814

Removed new inline annotations from declarations.

@EricWF How's the latest patch set looking? I believe the _LIBCPP_NEVER_INLINE_FOR_LARGE_CODEBASE is still required due to the linker limitation @pcc mentioned.

Any update on our plans to noinline symbols for large codebases?

thomasanderson abandoned this revision.Dec 27 2018, 12:34 PM