From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 309ED4696C3 for ; Tue, 14 Apr 2020 12:25:10 +0300 (MSK) Date: Tue, 14 Apr 2020 12:18:03 +0300 From: Igor Munkin Message-ID: <20200414091803.GJ5713@tarantool.org> References: <20200312100549.31608-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200312100549.31608-1-arkholga@tarantool.org> 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: Olga Arkhangelskaia Cc: tarantool-patches@dev.tarantool.org 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) > -- Best regards, IM