From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 A065D4696C3 for ; Tue, 14 Apr 2020 17:33:54 +0300 (MSK) References: <20200312100549.31608-1-arkholga@tarantool.org> <20200414091803.GJ5713@tarantool.org> From: Olga Arkhangelskaia Message-ID: Date: Tue, 14 Apr 2020 17:33:53 +0300 MIME-Version: 1.0 In-Reply-To: <20200414091803.GJ5713@tarantool.org> 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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Fixed tabs:  +            # Depending of compiler we need to set appropriate llvm/gcc-ar lib  +            # llvm/gcc -ar is just a wrapper over ar to pass --plugin= option,  +            # so that ar can understand LTO objects. 14.04.2020 12:18, Igor Munkin пишет: > Olya, > > Thanks for the patch! I left several comments below, please consider > them. > > On 12.03.20, Olga Arkhangelskaia wrote: >> 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+. >> >> Closes #3743 >> --- >> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci >> cmake/lto.cmake | 3 ++- >> cmake/luajit.cmake | 26 ++++++++++++++++++++++++++ >> 2 files changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/cmake/lto.cmake b/cmake/lto.cmake >> index 95ade75f4..9f29f3324 100644 >> --- a/cmake/lto.cmake >> +++ b/cmake/lto.cmake >> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN) >> if (linker_version VERSION_LESS "2.31") >> message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO") >> endif() >> - elseif(matched_gold) >> + >> + elseif(matched_gold) > Typo: there is a mess with whitespace above. > > However this chunk provides no changes except whitespace and newline. > Please drop it. > >> set(linker_version ${CMAKE_MATCH_1}) >> message(STATUS "Found ld.gold version: ${linker_version}") >> >> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake >> index 072db8269..da1b4926d 100644 >> --- a/cmake/luajit.cmake >> +++ b/cmake/luajit.cmake >> @@ -225,6 +225,32 @@ macro(luajit_build) >> set(luajit_xcflags ${luajit_xcflags} -D${def}) >> endforeach() >> >> + # 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 AND >> + NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4) > So, if clang toolchain is used but version is less than 3.4, then gcc > toolchain settings are used. It's either totally wrong or definitely > need to be clarified with a corresponding comment. > > Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported > CMake module. So additional check here looks to be excess or also need > to be clarified. > >> + if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9) >> + set(luajit_cflags ${luajit_cflags} -flto=full) >> + else() >> + set(luajit_cflags ${luajit_cflags} -flto=full) >> + endif() > I see no difference in the branches above. > >> + if (NOT TARGET_OS_DARWIN) >> + # Depending of compiler we need to set appropriate llvm/gcc-ar lib >> + set (CMAKE_AR llvm-ar) > What ar is used on systems different from Darwin based ones when clang > toolchain is used? > >> + endif() >> + # GNU opts to support lto >> + else() >> + #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+ >> + #The same is for binutils prior 2.27 >> + 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() >> + set (CMAKE_AR gcc-ar) > Could you please clarify why ar is set explicitly here? > >> + endif() >> + endif() > Typo: there is a mess with indent above. Unfortunately I failed to find > our CMake style guide, so please adjust your changes considering the > code nearby. > >> # 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 >> -- >> 2.20.1 (Apple Git-117) >>