Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia <arkholga@tarantool.org>]

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@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()