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

[LTO] Support LLVM level link time optimization on Darwin, Linux and Windows #31146

Merged
merged 1 commit into from
Jun 5, 2020
Merged

[LTO] Support LLVM level link time optimization on Darwin, Linux and Windows #31146

merged 1 commit into from
Jun 5, 2020

Conversation

kateinoigakukun
Copy link
Member

This commit adds -llvm-lto flag for driver to enable LTO at LLVM level.
When -llvm-lto given, compiler emits LLVM bitcode file instead of object
file and link them using libLTO.dylib plugin.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

@compnerd
Copy link
Member

What do you think of spelling this option as -lto=llvm? I don't think that it makes much sense to ever do the "full" LTO, doing thin is sufficient for nearly all the cases. If full is ever desired, it can be added as a longer spelling.

This will also allow for -lto=swift allowing the different LTO levels.

@compnerd
Copy link
Member

Why is this limited to macOS? Linux and Windows can also support this.

@kateinoigakukun kateinoigakukun force-pushed the katei/enable-lang-agnostic-lto branch 2 times, most recently from 7510a06 to 11a6113 Compare April 24, 2020 13:53
@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Apr 24, 2020 •
edited
Loading

What do you think of spelling this option as -lto=llvm? I don't think that it makes much sense to ever do the "full" LTO, doing thin is sufficient for nearly all the cases. If full is ever desired, it can be added as a longer spelling.

@compnerd I updated option style to -lto=llvm and supported thin LTO.
For -lto=llvm, perform thin LTO and for -lto=llvm-full, perform full LTO.

Why is this limited to macOS? Linux and Windows can also support this.

I tried to support Linux and Windows but I faced some problems I want to ask you.

1. For Linux:

LLVM LTO with gold needs LLVMgold.so plugin but currently Swift toolchain doesn't have the library.

LLVMgold.so is licensed under the GPLv3. Can we contain the library in Swift toolchain licensed under Apache License version 2.0?

Or should we give up to link using gold and use lld instead of gold on Linux?

2. For Windows:

(I'm not familiar with Windows development)

As far as I investigated, only lld supports LLVM LTO on Windows.
Should we use lld instead of default linker link?

@compnerd
Copy link
Member

LLVMgold is part of the LLVM project, it is not GPL, but Apache2 as the rest of LLVM: https://github.com/llvm/llvm-project/blob/master/llvm/tools/gold/gold-plugin.cpp#L1-L7

That said, I don't think that requiring lld is a terrible thing. Windows is going to require that we use lld anyways. link does support LTO, just does not have a plugin interface. It is enabled via the /LTCG flag.

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Apr 27, 2020 •
edited
Loading

@compnerd According to LLVM doc, LLVMgold binary is licensed under GPL. The gold-plugin.cpp is Apache2 but plugin-api.h included by gold-plugin.cpp is GPLv3. So the produced binary is GPLv3.

Anyway, I'm going to try to use LLVMgold on Linux and lld-link on Windows.

@kateinoigakukun kateinoigakukun force-pushed the katei/enable-lang-agnostic-lto branch 2 times, most recently from 4866f8c to 550fafe Compare April 27, 2020 14:22
@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Apr 27, 2020 •
edited
Loading

@compnerd I implemented for both Linux and Windows using LLVMgold and lld-link.

But I faced an issue on windows around static archives.
swiftc emits static archives with name prefixed with "lib". #25202 (comment)

But when clang linker received -lA, clang doesn't prepend "lib" prefix for actual library filename. https://github.com/llvm/llvm-project/blob/ff5264f0c6f026b25e2d91d0f5d5377a156c1f40/clang/lib/Driver/ToolChains/MSVC.cpp#L474-L481
I'm not familiar with Windows convention, so I want to ask you which naming rule we should follow.

@kateinoigakukun kateinoigakukun changed the title [LTO] Support LLVM level link time optimization on Darwin [LTO] Support LLVM level link time optimization on Darwin, Linux and Windows Apr 27, 2020
@compnerd
Copy link
Member

Static libraries shouldn’t have the lib prefix imo. However, static libraries don’t work on windows. I would recommend that you just stick to dynamic libraries everywhere.

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Apr 27, 2020 •
edited
Loading

However, static libraries don’t work on windows.

OK, I disabled some test cases related to static library on Windows

@kateinoigakukun kateinoigakukun force-pushed the katei/enable-lang-agnostic-lto branch 3 times, most recently from 5e4a643 to a6096da Compare April 27, 2020 15:31
@kateinoigakukun kateinoigakukun mentioned this pull request Apr 27, 2020
Closed
compnerd
compnerd requested changes May 2, 2020
View reviewed changes
include/swift/Driver/Action.h Outdated Show resolved Hide resolved
lib/Driver/DarwinToolChains.cpp Outdated Show resolved Hide resolved
lib/Driver/Driver.cpp Outdated Show resolved Hide resolved
lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved
tools/driver/autolink_extract_main.cpp Outdated Show resolved Hide resolved
utils/update_checkout/update-checkout-config.json Outdated Show resolved Hide resolved
compnerd
compnerd requested changes May 12, 2020
View reviewed changes
include/swift/Driver/Action.h Outdated Show resolved Hide resolved
include/swift/Driver/Action.h Outdated Show resolved Hide resolved
include/swift/Driver/Action.h Outdated Show resolved Hide resolved
@compnerd
Copy link
Member

I don't think that we should be cloning the binutils repository.

@kateinoigakukun kateinoigakukun force-pushed the katei/enable-lang-agnostic-lto branch 3 times, most recently from a476a27 to 89b1752 Compare May 13, 2020 13:08
@kateinoigakukun
Copy link
Member Author

@compnerd

I don't think that we should be cloning the binutils repository.

binutils repository is necessary to build LLVMgold.so. But I agree that we should use official repository instead of mirror repo if we use that.
Do you have a good idea?

@compnerd
Copy link
Member

You can require that the user install the package. What exactly is needed from the binutils package? The license in that makes it challenging to use.

@kateinoigakukun kateinoigakukun force-pushed the katei/enable-lang-agnostic-lto branch 2 times, most recently from 50eddff to c8bddd8 Compare May 13, 2020 20:03
@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented May 13, 2020 •
edited
Loading

@compnerd

What exactly is needed from the binutils package? The license in that makes it challenging to use.

LLVMGold depends on plugin-api.h in binutils repo according to http://llvm.org/docs/GoldPlugin.html#licensing.

You can require that the user install the package

OK, I removed binutils dependency. Now, user needs to build LLVMGold following docs and put it in the path/to/toolchain/usr/lib/.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 96ad8e541669d02907672f4702cd608120c3b6ca

@compnerd
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7ee923bef20bae23eb3bd6d8198e7aea28250a9e

@compnerd
Copy link
Member

@swift-ci please test

@kateinoigakukun
Copy link
Member Author

I found that lld is not built on windows CI, so the job failed. I'll add lld to LLVM_ENABLE_PROJECTS in build-windows.bat.

@kateinoigakukun
Copy link
Member Author

@compnerd Could you trigger CI again?

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7ee923bef20bae23eb3bd6d8198e7aea28250a9e

@compnerd
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 7ee923bef20bae23eb3bd6d8198e7aea28250a9e

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7ee923bef20bae23eb3bd6d8198e7aea28250a9e

@compnerd
Copy link
Member

@swift-ci please test Linux platform

@compnerd
Copy link
Member

@swift-ci please test Windows platform

@kateinoigakukun
[LTO] Support LLVM level link time optimization
915c4a6 
This commit adds -lto flag for driver to enable LTO at LLVM level.
When -lto=llvm given, compiler emits LLVM bitcode file instead of object
file and perform thin LTO using libLTO.dylib plugin.
When -lto=llvm-full given, perform full LTO instead of thin LTO.
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7297430f183d2b35238dfa0def48d3bde69c98e6

@compnerd
Copy link
Member

@swift-ci please clean test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 915c4a6

@compnerd
Copy link
Member

compnerd commented Jun 1, 2020

@swift-ci please test Windows platform

@compnerd
Copy link
Member

compnerd commented Jun 1, 2020

@swift-ci please test Linux platform

1 similar comment
@compnerd
Copy link
Member

compnerd commented Jun 2, 2020

@swift-ci please test Linux platform

@MaxDesiatov
Copy link
Contributor

Is there anything that prevents this PR from being merged?

@compnerd compnerd merged commit d0131ba into swiftlang:master Jun 5, 2020
@3405691582
Copy link
Contributor

On platforms which don't support LTO, should these just be XFAIL? Or is there a nicer way of detecting lack of LTO that we can put into a REQUIRES declaration?

@compnerd
Copy link
Member

compnerd commented Jun 7, 2020

All the ELF platforms should be able to support LTO, though it requires that lld is used as the linker. LTO doesn't really have much in terms of platform specific dependencies other than a linker that supports LTO itself.

@compnerd
Copy link
Member

compnerd commented Jun 7, 2020

@kateinoigakukun - this seems to have caused failures on many of the CI hosts due to the LLVMgold plugin not being built I suspect:

/home/ubuntu/android-ndk-r17/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/aarch64-linux-android/bin/ld.gold: error: /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/buildbot_linux/llvm-linux-x86_64/bin/../lib/LLVMgold.so: could not load plugin library: /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/buildbot_linux/llvm-linux-x86_64/bin/../lib/LLVMgold.so: cannot open shared object file: No such file or directory

I'm going to revert this until that has been addressed.

@compnerd compnerd mentioned this pull request Jun 7, 2020
Merged
This was referenced Jun 7, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov left review comments

@compnerd compnerd compnerd approved these changes

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@kateinoigakukun @compnerd @swift-ci @shahmishal @MaxDesiatov @3405691582