From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 64CEA4696C3 for ; Fri, 17 Apr 2020 17:41:48 +0300 (MSK) References: <20200312100549.31608-1-arkholga@tarantool.org> <20200414091803.GJ5713@tarantool.org> <184a4359-9531-a35e-b77a-554394346cdd@tarantool.org> <20200414170024.GL5713@tarantool.org> <20200417114720.GK8314@tarantool.org> From: Olga Arkhangelskaia Message-ID: <30019d9a-bf60-eff2-9628-5346a76f9953@tarantool.org> Date: Fri, 17 Apr 2020 17:41:47 +0300 MIME-Version: 1.0 In-Reply-To: <20200417114720.GK8314@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 Hi Igor! It seems that I eventually had fixed white-space mess. While testing everything once again I came across reason why I separated case with Darwin and llvm-ar. Fixed and added a comment. Diffs are below: P.S. Please, let me know whether new diff is better. 17.04.2020 14:47, Igor Munkin пишет: > 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 @@ -233,27 +233,29 @@ macro(luajit_build)   6              if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)   7                  set (luajit_cflags ${luajit_cflags} -flto=full)   8 else()   9 -`......`.......# ThinLTO that is both scalable and incremental  10 -`......`.......# due to parallel IPO, available since 3.9 and above.  11 -`......`.......# See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html  12 -`......`.......set (luajit_cflags ${luajit_cflags} -flto=thin)  13 +                # ThinLTO that is both scalable and incremental  14 +                # due to parallel IPO, available since 3.9 and above.  15 +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html  16 +                set (luajit_cflags ${luajit_cflags} -flto=thin)  17 endif()  18 -            # Depending of compiler we need to set appropriate llvm/gcc-ar lib  19 -            # llvm/gcc -ar is just a wrapper over ar to pass --plugin= option,  20 -            # so that ar can understand LTO objects.  21 -            # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html~  22 -`......    set (CMAKE_AR llvm-ar)  23 +                # Since Darwin uses DYLD_LIBRARY_PATH instead of llvm-ar  24 +                # ar should not be change.  25 +                # See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/156799.html  26 +                if (NOT TARGET_OS_DARWIN)  27 +                    # llvm-ar is just a wrapper over ar to pass --plugin= option,  28 +                    # so that ar can understand LTO objects.  29 +                    # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html  30 +                    set (CMAKE_AR llvm-ar)  31 + endif()  32 else()  33 -`......# GNU opts to support lto  34 -`......`.......# Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+  35 -`......`.......# The same is for binutils prior 2.27  36 -`......`.......# See https://patchwork.kernel.org/patch/10078207/  37 +        # GNU opts to support lto  38 +            # Due to some problems (bugs, slow work, etc) we support LTO  39 +            # only for 5.0+. The same is for binutils prior to 2.27  40 +            # See comments in scripts/Makefile.lto in scope of  41 +            # the patch: https://patchwork.kernel.org/patch/10078207/  42              if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0  43                  AND NOT ${linker_version} VERSION_LESS 2.27)  44                  set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)  45 -            endif()  46 -`......    # The same as for llvm-ar  47 -`......    # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826  48 + endif()  49 +            # gcc-ar is just a wrapper over ar to pass --plugin option  50 +            # so ar can understand LTO objects. For further info see  51 +            # -flto and -ffat-lto-objects options descriptions:  52 +            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html  53              set (CMAKE_AR gcc-ar)  54 endif()  55      endif()