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.

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

[RDY] Add LTO support #8654

Merged
merged 3 commits into from
Jul 23, 2018
Merged

[RDY] Add LTO support #8654

merged 3 commits into from
Jul 23, 2018

Conversation

zhou13
Copy link
Contributor

@zhou13 zhou13 commented Jun 28, 2018

No description provided.

CMakeLists.txt Outdated
@@ -2,7 +2,7 @@
# intro: https://codingnest.com/basic-cmake/
# best practices (3.0+): https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1

cmake_minimum_required(VERSION 2.8.12)
cmake_minimum_required(VERSION 3.9)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to use 2.8.12? We need to be able to build on as many systems as possible.

@marvim marvim added the RFC label Jun 28, 2018
@zhou13
Copy link
Contributor Author

zhou13 commented Jun 29, 2018

@justinmk Updated.

@justinmk
Copy link
Member

justinmk commented Jul 1, 2018

We tried LTO in #4313 , later reverted in mgraczyk@c2b2e0e .

But it looks like you did something like what was discussed in #4313 (comment) , which was never tried.

@mhinz @Yamakaky @ZyX-I @jamessan @jszakmeister any comments/objections?

AppVeyor build had some weird failures; restarted.

@zhou13
Copy link
Contributor Author

zhou13 commented Jul 1, 2018

The new CMake policy CMP0069 has much better LTO support for compilers other than intel. It was introduced in CMake 3.9. This patch only enables LTO if both the CMake and the compiler support LTO/CMP0069.

@mhinz
Copy link
Member

mhinz commented Jul 2, 2018

What would be the exact advantage for Neovim? Is there a benchmark that shows an improvement in any way?

Does anyone know how to check if Mach-O executable was compiled with LTO? (clang or gcc)

@jamessan
Copy link
Member

jamessan commented Jul 2, 2018

What would be the exact advantage for Neovim?

The same sorts of advantages that come from compiling with -O2 rather than -O0, except that with LTO the compiler is able to make optimizations across translation units.

Does anyone know how to check if Mach-O executable was compiled with LTO?

There's no marker for the compiler using LTO. It's sort of doing another compilation pass but with all of the intermediate files available, which is why the linker needs the same optimization flags as the original compile.

@KillTheMule
Copy link
Contributor

KillTheMule commented Jul 2, 2018

As a semi-related note, in my little rust project adding lto has had a pretty serious performance impact. Benchmarks would be best, of course.

@mhinz
Copy link
Member

mhinz commented Jul 2, 2018

@jamessan

Ah, okay. I read that gcc added a .gnu.lto section header to an ELF and hoped it (or clang) would do something similar for Mach-O.

Best answer I found: https://stackoverflow.com/questions/51048414/clang-how-to-check-if-lto-was-performed

Well, LGTM.

@justinmk
Copy link
Member

justinmk commented Jul 2, 2018

MINGW_64 build failed. Restarted. If it fails again, @zhou13 can you try disabling LTO for mingw (by default)?

@zhou13
Copy link
Contributor Author

zhou13 commented Jul 3, 2018

MINGW_64 build seems to be OK. Failed on test.

@justinmk
Copy link
Member

justinmk commented Jul 3, 2018

Restarted AppVeyor again. It's very unusual for the AppVeyor build to fail, so I'm hesitant to merge this until it succeeds.

Can you try disabling LTO for mingw, to see if that allows appveyor to succeed?

@ZviRackover
Copy link
Contributor

Seems that Mingw32 passed while Mingw64 failed with a bunch of "plugin needed to handle lto object" errors.
FWIW, this is a similar situation to the StackOverflow question https://stackoverflow.com/questions/32221221/mingw-x64-windows-plugin-needed-to-handle-lto-object which has a solution to the issue.

@zhou13
Copy link
Contributor Author

zhou13 commented Jul 16, 2018

@ZviRackover Thanks for finding the clues!

@justinmk Should I just disable LTO for mingw, or add put the LTO plugin into the bfd-plugins directory? I don't think I can change the appveyor's CI.

@justinmk
Copy link
Member

justinmk commented Jul 17, 2018

Should I just disable LTO for mingw,

Let's do that for now. Just add a one-line comment above the line in CMakeLists.txt, something like this:

# Disable LTO for mingw.  https://github.com/Alexpux/MINGW-packages/issues/3516

( https://github.com/Alexpux/MINGW-packages/issues/3516 has better info than the SO post. )

@zhou13 zhou13 changed the title [RFC] Add LTO support [RDY] Add LTO support Jul 22, 2018
@zhou13
Copy link
Contributor Author

zhou13 commented Jul 22, 2018

Updated. Waiting for the CI.

@marvim marvim added RDY and removed RFC labels Jul 23, 2018
@justinmk justinmk merged commit c8e7a44 into neovim:master Jul 23, 2018
@justinmk justinmk removed the RDY label Jul 23, 2018
@justinmk
Copy link
Member

Thanks @zhou13 . I plan to start offering the MSVC builds in the releases instead of the mingw builds, so please report any quirks ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants