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

[Igor Munkin <imun@tarantool.org>]

Sasha,

Again, thanks for such deep research!

On 16.06.20, Alexander Turenko wrote:
> 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?

I'm not such a CMake expert and your patch is a black voodoo magic for
me. I guess Timur is the right person to ask regarding this approach,
considering his CMake experience (Cced him).

> 
> 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.

Yes, this is a "Poor Man's LTO" (I looked at it sometime ago) and we
discussed this approach offline with Sergey B. I don't undertake to
recommend this build type, due to exact the same reason you faced above:
| $ nm -g ./third_party/luajit/src/libluajit.a | grep -c lj_state_growstack1
| 0

Side note: Heh, this where internal API usage leads to. Hope, we'll fix
it one day.

> 
> 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].

I was curious but made no measurement by myself (neither size-related
nor performance-related). I discussed with Sergey offline and we ended
up with the fact this is much closer to compiler testing than to the
patch itself. However, it would be nice to provide some numbers to see
whether this patch has a value.

> 
> 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.

Agree.

> 
> > +        # 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.

OMG, another fun with MacOS support...

> 
> > +        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.

-- 
Best regards,
IM