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

[Igor Munkin <imun@tarantool.org>]

Olya,

Thanks for the patch! I left several comments below, please consider
them.

On 12.03.20, Olga Arkhangelskaia wrote:
> 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+.
> 
> Closes #3743
> ---
> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>  cmake/lto.cmake    |  3 ++-
>  cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
> index 95ade75f4..9f29f3324 100644
> --- a/cmake/lto.cmake
> +++ b/cmake/lto.cmake
> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>          if (linker_version VERSION_LESS "2.31")
>              message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>          endif()
> -    elseif(matched_gold)
> +
> +	elseif(matched_gold)

Typo: there is a mess with whitespace above.

However this chunk provides no changes except whitespace and newline.
Please drop it.

>          set(linker_version ${CMAKE_MATCH_1})
>          message(STATUS "Found ld.gold version: ${linker_version}")
>  
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 072db8269..da1b4926d 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -225,6 +225,32 @@ 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 AND
> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)

So, if clang toolchain is used but version is less than 3.4, then gcc
toolchain settings are used. It's either totally wrong or definitely
need to be clarified with a corresponding comment.

Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
CMake module. So additional check here looks to be excess or also need
to be clarified.

> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +		set(luajit_cflags ${luajit_cflags} -flto=full)
> +	    else()
> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
> +	    endif()

I see no difference in the branches above.

> +	if (NOT TARGET_OS_DARWIN)
> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
> +	   set (CMAKE_AR llvm-ar)

What ar is used on systems different from Darwin based ones when clang
toolchain is used?

> +	endif()
> +	# GNU opts to support lto
> +	else()
> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
> +	   #The same is for binutils prior 2.27
> +	   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()
> +	   set (CMAKE_AR gcc-ar)

Could you please clarify why ar is set explicitly here?

> +        endif()
> +    endif()

Typo: there is a mess with indent above. Unfortunately I failed to find
our CMake style guide, so please adjust your changes considering the
code nearby.

>      # 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
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM