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

[Olga Arkhangelskaia <arkholga@tarantool.org>]

Update branch and checked output on release_lto and release_lto_clang8.

They are ok.

16.06.2020 14:36, Olga Arkhangelskaia пишет:
> 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.