Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add LTO support (full and thin), with -flto=thin|full. #1840

Merged
merged 2 commits into from
Nov 9, 2016
Merged

Add LTO support (full and thin), with -flto=thin|full. #1840

merged 2 commits into from
Nov 9, 2016

Conversation

JohanEngelen
Copy link
Member

@JohanEngelen JohanEngelen commented Oct 19, 2016 •
edited
Loading

LTO needs linker support: I am only aware of support on OS X and Linux (through the LLVMgold plugin).

I chose the cmdline option flto=thin|full that mimics Clang's option (also Martin was looking for flto in the issue #693 he reported).

Resolves #693 .

For OS X, additional logic could be added later to point to a newer libLTO.dylib (see clang's darwin::Linker::AddLinkArgs). Done: added flto-binary for that purpose. Also, CMake installs LLVM's LTO libs when it can find them; the LTO lib is installed in LDC's /lib/, and that's the first path searched when adding LTO linker flags.

@MartinNowak
Copy link
Contributor

Yes, using clang's options seems like the obvious choice.
clang --help | grep -i lto

  -flto=<value>           Set LTO mode to either 'full' or 'thin'
  -flto                   Enable LTO in 'full' mode
  -fno-lto                Disable LTO mode (default)
  -fthinlto-index=<value> Perform ThinLTO importing using provided function summary index

Only have a vague idea what this fthinlto-index option is, also see [ThinLTO] Option to invoke ThinLTO backend passes and importing.

dnadlinger
dnadlinger reviewed Oct 21, 2016
View reviewed changes
driver/linker.cpp

unsigned char opt = optLevel();
static char optChars[15] = "-plugin-opt=O0";
optChars[13] = '0' + (opt & 0x03); // the plugin O-level ranges from 0 to 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered just making this min(optLevel(), 3)? -O4 and -O5 are currently documented as being equal to -O3, and it doesn't seem like resetting to 0 resp. 1 would be intuitive for these.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How stupid, thanks for catching this bug. Trying to optimize something that doesn't need optimizing.

driver/linker.cpp
namespace {

std::string getLTOGoldPluginPath() {
// TODO: cmdline option for plugin path, or in ldc.conf?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command-line option which can then be set from ldc2.conf? ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: the plugin path is also useful for OS X.

@JohanEngelen
Copy link
Member Author

@MartinNowak See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html for -fthinlto-index . I'm leaving that out for now.

@JohanEngelen JohanEngelen force-pushed the LTO branch 3 times, most recently from afa648b to fece3f9 Compare October 23, 2016 14:23
@JohanEngelen
Copy link
Member Author

JohanEngelen commented Oct 24, 2016 •
edited
Loading

Bug : When there are module ctors/dtors in different modules, not all of them are run :(

Edit: I think I found the cause. There is a latent bug in our llvm.used emission. ThinLTO is very aggressively optimizing and thus things break. Working on a fix. (edit: #1855)

Edit2: nice! With that fix, I managed to build LDC with LTO, mixing C++ and D LTO! In other words: cross-language cross-module inlining.

@kinke
Copy link
Member

kinke commented Oct 24, 2016

mixing C++ and D LTO

Pretty cool. :)

@JohanEngelen JohanEngelen force-pushed the LTO branch 2 times, most recently from e9e291e to 756fd59 Compare October 27, 2016 12:22
@JohanEngelen
Copy link
Member Author

LTO testing is now enabled on CircleCI

@JohanEngelen
Copy link
Member Author

Any concerns before I merge this?

@thewilsonator thewilsonator mentioned this pull request Nov 6, 2016
Merged
@kinke kinke mentioned this pull request Nov 7, 2016
Closed
@JohanEngelen
Add LTO support (full and thin), with -flto=thin|full.
bb24fb2 
LTO needs linker support: I am only aware of support on OS X and Linux (through the LLVMgold plugin).

Resolves ldc-developers#693
@JohanEngelen
[CircleCI] use ld.gold per default to enable LTO testing
b15bb73
@JohanEngelen
Copy link
Member Author

Merging when green.

@JohanEngelen JohanEngelen merged commit 3722caa into ldc-developers:master Nov 9, 2016
@JohanEngelen JohanEngelen deleted the LTO branch November 10, 2016 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnadlinger dnadlinger dnadlinger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JohanEngelen @MartinNowak @kinke @dnadlinger