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 46C164696C3 for ; Fri, 17 Apr 2020 14:54:27 +0300 (MSK) Date: Fri, 17 Apr 2020 14:47:20 +0300 From: Igor Munkin Message-ID: <20200417114720.GK8314@tarantool.org> References: <20200312100549.31608-1-arkholga@tarantool.org> <20200414091803.GJ5713@tarantool.org> <184a4359-9531-a35e-b77a-554394346cdd@tarantool.org> <20200414170024.GL5713@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 fixes! On 15.04.20, Olga Arkhangelskaia wrote: > Hi Igor! > > See diff below: > > 14.04.2020 20:00, Igor Munkin пишет: Sorry, but I failed to read the diff you placed at the end. Since I'm using the same editor you do (vim, right?) let me share with you the recipe I use for that purposes: 1. Save the diff you're going to paste to a temporary file. 2. Move the cursor to the place you're going to place the diff. 3. Run the following command: | :r I placed the diff from the upstream branch below. The wording now is definitely the way much better, thanks! However, please consider several nits I left inline. Unfortunately, there is still a mess with a whitespace. Please fix it. ================================================================================ From: Olga Arkhangelskaia Subject: [PATCH] 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 --- cmake/luajit.cmake | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake index 072db8269..a0d2768b2 100644 --- a/cmake/luajit.cmake +++ b/cmake/luajit.cmake @@ -225,6 +225,38 @@ 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) + 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 Side note: great and clear wording, thanks! + set (luajit_cflags ${luajit_cflags} -flto=thin) + endif() + # Depending of compiler we need to set appropriate llvm/gcc-ar lib Typo: s/Depending of/Depending on/. + # llvm/gcc -ar is just a wrapper over ar to pass --plugin= option, I guess we can omit all gcc mentions within this comment and move all gcc-related specifics below. + # so that ar can understand LTO objects. + # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html Typo: trailing whitespace above. + set (CMAKE_AR llvm-ar) + else() + # GNU opts to support lto + # Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+ Typo: Line exceeds 80 symbols (furthermore, AFAIR comments are limited with 66 symbols, but I don't know whether this limit relates to CMake sources). + # The same is for binutils prior 2.27 Typo: s/prior/prior to/. + # See https://patchwork.kernel.org/patch/10078207/ Minor: It's a well structured long-read. I propose to point rationale the following way (leaving other lines as is): | # 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() + # The same as for llvm-ar + # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826 GGC specifics are better described here[1]. So the comment can be reworded the following way: | # 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 Typo: an excess comma. + 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 -- 2.25.0 ================================================================================ I see the tests are failed. The following jobs failed due to flaky tests, so I just restarted them: * release_clang[2] * release_asan_clang8[3] * osx_13_release[4] However, there are some builds failed due to the changes: * osx_14_release_lto[5]: | AR libluajit.a | make[4]: llvm-ar: No such file or directory There are also failed jobs that are removed in bleeding master (Ubuntu 18.10 and Ubuntu 19.04 jobs are removed considering distro version EOL). Please rebase you branch on the master branch to fix this issue. > [1]: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html [1]: https://gitlab.com/tarantool/tarantool/-/jobs/515804361 [2]: https://gitlab.com/tarantool/tarantool/-/jobs/515812948 [3]: https://gitlab.com/tarantool/tarantool/-/jobs/515827993 [3]: https://gitlab.com/tarantool/tarantool/-/jobs/512029069#L1912 -- Best regards, IM