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

[Olga Arkhangelskaia <arkholga@tarantool.org>]

Hi!

Alexander! I am very thankful for the detailed review of the issue.

I have spent a lot of time reading about LTO and how it is supported in 
others projects. Ad did not

think about cmake itself.

I have applied your patch and edit luajit.cmake according to newly 
exposed variables. It has changed completely.

Diff:

-    # 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
-                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()
- 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()
+    # Enablibg LTO for luajit if DENABLE_LTO set.
+    if (${ENABLE_LTO})
+`......message(STATUS "Enablig LTO for luajit")
+        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})
+`......set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
+`......set (CMAKE_AR ${AR_LTO})
endif()
-    # Pass the same toolchain that is used for building of

I have test it with MacOS  and debian. And will do full-ci for it.

16.06.2020 4:02, Alexander Turenko пишет:
> 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.