From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 1BB1342EF5C for ; Tue, 16 Jun 2020 15:01:09 +0300 (MSK) From: Olga Arkhangelskaia References: <20200312100549.31608-1-arkholga@tarantool.org> <20200616010232.3j2zvwo7z6lmnody@tkn_work_nb> Message-ID: <9f2e5ad2-dad0-3b60-4f62-6c5550904fc6@tarantool.org> Date: Tue, 16 Jun 2020 15:01:07 +0300 MIME-Version: 1.0 In-Reply-To: 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 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 >>> 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.