From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E6BD942EF5C for ; Tue, 16 Jun 2020 14:36:22 +0300 (MSK) References: <20200312100549.31608-1-arkholga@tarantool.org> <20200616010232.3j2zvwo7z6lmnody@tkn_work_nb> From: Olga Arkhangelskaia Message-ID: Date: Tue, 16 Jun 2020 14:36:15 +0300 MIME-Version: 1.0 In-Reply-To: <20200616010232.3j2zvwo7z6lmnody@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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 >> 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 " > | ) > | > | set(CMAKE_${lang}_ARCHIVE_APPEND_IPO > | "\"${__ar}\" r " > | ) > | > | set(CMAKE_${lang}_ARCHIVE_FINISH_IPO > | "\"${__ranlib}\" " > | ) > > 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.