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

[Alexander Turenko <alexander.turenko@tarantool.org>]

I vote to use the same build tools and flags that CMake uses to build
files under its control, because otherwise we would write the same logic
again. It may be not as accurate as one that CMake provides (see example
below). It may become outdated in a future. And, last but not least,
duplicated code is painful to maintain.

However the way I proposed requires to use undocumented CMake variables
(see [0]). But I think that's evil of the lesser kind.

I think it would be good to expose related build tool names and flags
from cmake/lto.cmake and use them in cmake/luajit.cmake. I implemented
the former part (I would even left STATUS messages as is, they provide
useful information):

 | diff --git a/cmake/lto.cmake b/cmake/lto.cmake
 | index 95ade75f4..79b908e26 100644
 | --- a/cmake/lto.cmake
 | +++ b/cmake/lto.cmake
 | @@ -90,8 +90,40 @@ if (NOT TARGET_OS_DARWIN)
 |      endif()
 |  endif()
 |  
 | -# gh-3742: investigate LTO warnings.
 | -set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
 | +# {{{ Expose build tools and flags
 | +#
 | +# It is convenient for building non-cmake targets with the same
 | +# flags as we use for sources under CMake control.
 | +#
 | +# It leans on uncodumented variables that are set in the following
 | +# CMake modules: Compiler/GNU.cmake and Compiler/Clang.cmake.
 | +
 | +# CFLAGS_LTO (list)
 | +set(CFLAGS_LTO ${CMAKE_C_COMPILE_OPTIONS_IPO})
 | +message(STATUS "CFLAGS_LTO: ${CFLAGS_LTO}")
 |  
 | +# LDFLAGS_LTO (list)
 | +set(LDFLAGS_LTO ${CMAKE_C_LINK_OPTIONS_IPO})
 | +# FIXME: gh-3742: investigate LTO warnings.
 | +list(APPEND LDFLAGS_LTO -Wno-lto-type-mismatch)
 | +message(STATUS "LDFLAGS_LTO: ${LDFLAGS_LTO}")
 | +
 | +# AR_LTO (string)
 | +#
 | +# Note: Platform/Linux-Intel.cmake and Platform/Windows-MSVC.cmake
 | +# set CMAKE_C_CREATE_STATIC_LIBRARY_IPO, but not
 | +# CMAKE_C_ARCHIVE_CREATE_IPO. So this snippet is only for GCC and
 | +# clang.
 | +set(_ar_command ${CMAKE_C_ARCHIVE_CREATE_IPO})
 | +separate_arguments(_ar_command)
 | +list(GET _ar_command 0 AR_LTO)
 | +unset(_ar_command)
 | +message(STATUS "AR_LTO: ${AR_LTO}")
 | +
 | +# }}}
 | +
 | +# Set build tools and flags for files that are built using CMake.
 | +set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
 |  set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
 | +
 |  message(STATUS "Enabling LTO: TRUE")

Olga, Igor, what do you think about this way?

Or, maybe, we should just add LJCORE_O=ljamalg.o to the Make command
(just like `make amalg` does)? I was curious and tried it:

 | diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
 | index dd9d748ca..d3df66652 100644
 | --- a/cmake/luajit.cmake
 | +++ b/cmake/luajit.cmake
 | @@ -284,6 +284,7 @@ macro(luajit_build)
 |          CCOPT="${luajit_ccopt}"
 |          CCDEBUG="${luajit_ccdebug}"
 |          XCFLAGS="${luajit_xcflags}"
 | +        LJCORE_O=ljamalg.o
 |          Q=''
 |          # We need to set MACOSX_DEPLOYMENT_TARGET to at least 10.6,
 |          # because 10.4 SDK (which is set by default in LuaJIT's

And got undefined reference errors from src/lua/utils.c code that use
LuaJIT internals.

Despite way we'll choose, I would try some benchmarks with jit enabled
and disabled (`jit.off()`) to see whether compiling of LuaJIT with LTO
gives some gains. Technically it is not part of the task (my bad), so
skip it if you're not curious. There is the good candidate to look into:
[3].

Placed the actual version from the branch [1] to comment it.

> commit 62e5d0af7f7a5e44f27b741d43c6c035f20e93b4
> Author: Olga Arkhangelskaia <arkholga@tarantool.org>
> Date:   Tue Mar 3 12:05:49 2020 +0300
> 
>     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
> 
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 10df633d5..dd9d748ca 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -224,6 +224,45 @@ macro(luajit_build)
>          set(luajit_xcflags ${luajit_xcflags} -D${def})
>      endforeach()
>  
> +    # Add LTO option to luajit
> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)

I would lean on ENABLE_LTO variable and the fact that all necessary
checks already performed by cmake/lto.cmake. I guess it would be more
intuitive.

> +        message("Setting LTO flags for building luajit")

I would use STATUS mode, the message is informational and should not
attract user's attention. STATUS mode is used for such messages in
cmake/lto.cmake. See [2] for information about message types.

> +        # 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
> +                set (luajit_cflags ${luajit_cflags} -flto=thin)
> +            endif()
> +            # XCode linker dynamically load libLTO.dylib to perform
> +            # link-time optimization.
> +            # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> +            if (NOT TARGET_OS_DARWIN)
> +                # llvm-ar is just a wrapper over ar to pass --plugin= option,
> +                # so that ar can understand LTO objects.
> +                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> +                set (CMAKE_AR llvm-ar)
> +            endif()

I guess it reflects the logic of the following snippet from
Compiler/Clang.cmake CMake module:

 | string(COMPARE EQUAL "${CMAKE_${lang}_COMPILER_ID}" "AppleClang" __is_apple_clang)
 | <...>
 | if(ANDROID OR __is_apple_clang)
 |   set(__ar "${CMAKE_AR}")
 |   set(__ranlib "${CMAKE_RANLIB}")
 | else()
 |   set(__ar "${CMAKE_${lang}_COMPILER_AR}")
 |   set(__ranlib "${CMAKE_${lang}_COMPILER_RANLIB}")
 | endif()
 |
 | set(CMAKE_${lang}_ARCHIVE_CREATE_IPO
 |   "\"${__ar}\" cr <TARGET> <LINK_FLAGS> <OBJECTS>"
 | )
 |
 | set(CMAKE_${lang}_ARCHIVE_APPEND_IPO
 |   "\"${__ar}\" r <TARGET> <LINK_FLAGS> <OBJECTS>"
 | )
 |
 | set(CMAKE_${lang}_ARCHIVE_FINISH_IPO
 |   "\"${__ranlib}\" <TARGET>"
 | )

So TARGET_OS_DARWIN check is not accurate: we should check whether Clang
is built by Apple or, say, installed from brew; not just OS.

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

[0]: https://gitlab.kitware.com/cmake/cmake/-/issues/16808
[1]: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
[2]: https://cmake.org/cmake/help/latest/command/message.html
[3]: https://gitspartv.github.io/LuaJIT-Benchmarks/

WBR, Alexander Turenko.