[Tarantool-patches] [PATCH] cmake: add LTO support for building luajit
Olga Arkhangelskaia
arkholga at tarantool.org
Fri Apr 17 17:41:47 MSK 2020
Hi Igor!
It seems that I eventually had fixed white-space mess.
While testing everything once again I came across reason why I separated
case with Darwin and
llvm-ar. Fixed and added a comment.
Diffs are below:
P.S.
Please, let me know whether new diff is better.
17.04.2020 14:47, Igor Munkin пишет:
> Olya,
>
> Thanks for the fixes!
>
> On 15.04.20, Olga Arkhangelskaia wrote:
>> Hi Igor!
>>
>> See diff below:
>>
>> 14.04.2020 20:00, Igor Munkin пишет:
> <snipped>
>
> Sorry, but I failed to read the diff you placed at the end. Since I'm
> using the same editor you do (vim, right?) let me share with you the
> recipe I use for that purposes:
>
> 1. Save the diff you're going to paste to a temporary file.
> 2. Move the cursor to the place you're going to place the diff.
> 3. Run the following command:
> | :r <diff/file/name>
>
> I placed the diff from the upstream branch below.
>
> The wording now is definitely the way much better, thanks! However,
> please consider several nits I left inline.
>
> Unfortunately, there is still a mess with a whitespace. Please fix it.
>
> ================================================================================
>
> From: Olga Arkhangelskaia <arkholga at tarantool.org>
> Subject: [PATCH] cmake: add LTO support for building luajit
>
> Tarantool has LTO support, however while building luajit this opt. was
> omitted. Patch adds necessary flag to turn it on.
>
> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
> errors and slow work.
>
> Closes #3743
> ---
> cmake/luajit.cmake | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 072db8269..a0d2768b2 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -225,6 +225,38 @@ macro(luajit_build)
> set(luajit_xcflags ${luajit_xcflags} -D${def})
> endforeach()
>
> + # Add LTO option to luajit
> + if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> + message("Setting LTO flags for building luajit")
> + # Clang opt to support LTO
> + if (CMAKE_COMPILER_IS_CLANG)
> + if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> + set (luajit_cflags ${luajit_cflags} -flto=full)
> + else()
> + # ThinLTO that is both scalable and incremental
> + # due to parallel IPO, available since 3.9 and above.
> + # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
>
> Side note: great and clear wording, thanks!
>
> + set (luajit_cflags ${luajit_cflags} -flto=thin)
> + endif()
> + # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>
> Typo: s/Depending of/Depending on/.
>
> + # llvm/gcc -ar is just a wrapper over ar to pass --plugin= option,
>
> I guess we can omit all gcc mentions within this comment and move all
> gcc-related specifics below.
>
> + # so that ar can understand LTO objects.
> + # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
>
> Typo: trailing whitespace above.
>
> + set (CMAKE_AR llvm-ar)
> + else()
> + # GNU opts to support lto
> + # Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>
> Typo: Line exceeds 80 symbols (furthermore, AFAIR comments are limited
> with 66 symbols, but I don't know whether this limit relates to CMake
> sources).
>
> + # The same is for binutils prior 2.27
>
> Typo: s/prior/prior to/.
>
> + # See https://patchwork.kernel.org/patch/10078207/
>
> Minor: It's a well structured long-read. I propose to point rationale
> the following way (leaving other lines as is):
> | # See comments in scripts/Makefile.lto in scope of
> | # the patch: https://patchwork.kernel.org/patch/10078207/
>
> + if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> + AND NOT ${linker_version} VERSION_LESS 2.27)
> + set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> + endif()
> + # The same as for llvm-ar
> + # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826
>
> GGC specifics are better described here[1]. So the comment can be
> reworded the following way:
> | # gcc-ar is just a wrapper over ar to pass --plugin option
> | # so ar can understand LTO objects. For further info see
> | # -flto and -ffat-lto-objects options descriptions:
> | # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
>
> Typo: an excess comma.
>
> + set (CMAKE_AR gcc-ar)
> + endif()
> + endif()
> # Pass the same toolchain that is used for building of
> # tarantool itself, because tools from different toolchains
> # can be incompatible. A compiler and a linker are already set
@@ -233,27 +233,29 @@ macro(luajit_build)
6 if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
7 set (luajit_cflags ${luajit_cflags} -flto=full)
8 else()
9 -`......`.......# ThinLTO that is both scalable and incremental
10 -`......`.......# due to parallel IPO, available since 3.9 and above.
11 -`......`.......# See
http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
12 -`......`.......set (luajit_cflags ${luajit_cflags} -flto=thin)
13 + # ThinLTO that is both scalable and incremental
14 + # due to parallel IPO, available since 3.9 and above.
15 + # See
http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
16 + set (luajit_cflags ${luajit_cflags} -flto=thin)
17 endif()
18 - # Depending of compiler we need to set appropriate
llvm/gcc-ar lib
19 - # llvm/gcc -ar is just a wrapper over ar to pass
--plugin= option,
20 - # so that ar can understand LTO objects.
21 - # See
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html~
22 -`...... set (CMAKE_AR llvm-ar)
23 + # Since Darwin uses DYLD_LIBRARY_PATH instead of
llvm-ar
24 + # ar should not be change.
25 + # See
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/156799.html
26 + if (NOT TARGET_OS_DARWIN)
27 + # llvm-ar is just a wrapper over ar to pass
--plugin= option,
28 + # so that ar can understand LTO objects.
29 + # See
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
30 + set (CMAKE_AR llvm-ar)
31 + endif()
32 else()
33 -`......# GNU opts to support lto
34 -`......`.......# Due to some problems (bugs, slow work, etc) we
support LTO only for 5.0+
35 -`......`.......# The same is for binutils prior 2.27
36 -`......`.......# See https://patchwork.kernel.org/patch/10078207/
37 + # GNU opts to support lto
38 + # Due to some problems (bugs, slow work, etc) we
support LTO
39 + # only for 5.0+. The same is for binutils prior to 2.27
40 + # See comments in scripts/Makefile.lto in scope of
41 + # the patch: https://patchwork.kernel.org/patch/10078207/
42 if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
43 AND NOT ${linker_version} VERSION_LESS 2.27)
44 set (luajit_cflags ${luajit_cflags} -flto
-fuse-linker-plugin -fno-fat-lto-objects)
45 - endif()
46 -`...... # The same as for llvm-ar
47 -`...... # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826
48 + endif()
49 + # gcc-ar is just a wrapper over ar to pass --plugin
option
50 + # so ar can understand LTO objects. For further info see
51 + # -flto and -ffat-lto-objects options descriptions:
52 + #
https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
53 set (CMAKE_AR gcc-ar)
54 endif()
55 endif()
More information about the Tarantool-patches
mailing list