From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2CAC22CDEB for ; Thu, 11 Oct 2018 12:01:37 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id eJaK7JGZsvjY for ; Thu, 11 Oct 2018 12:01:37 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 575EB2CDCB for ; Thu, 11 Oct 2018 12:01:36 -0400 (EDT) From: Alex Khatskevich Subject: [tarantool-patches] Re: [PATCH 2/3] Add LTO support References: <64060dd168d3977f90f06b0bd242cf7ecc256a07.1533726342.git.avkhatskevich@tarantool.org> <20181010142948.nlwmr4bz77okripx@tkn_work_nb> Message-ID: <90f88757-12a4-be78-7ef3-d7b3a7ca97a2@tarantool.org> Date: Thu, 11 Oct 2018 19:01:31 +0300 MIME-Version: 1.0 In-Reply-To: <20181010142948.nlwmr4bz77okripx@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Alexander Turenko Cc: georgy@tarantool.org, tarantool-patches@freelists.org On 10.10.2018 17:29, Alexander Turenko wrote: > On Wed, Aug 08, 2018 at 02:10:02PM +0300, AKhatskevich wrote: >> This commit introduces LTO support. >> >> Changes: >> - update submodules (fix lto-related warnings) > The msgpack submodule update contains a commit that are not related to > your change. We should not hide such changes in the commit history. > Maybe it is better to update submodules in a separate commit with proper > description of all changes it pulls into the main repository. That commit is just a testcase fix. Nothing special. >> - add `TARANTOOL_LTO` cmake option (by default it is `False`) > It seems such options usually looks like ENABLE_XXX=[ON/OFF]. Renamed to ENABLE_LTO >> LTO speeds up cpu-intensive workloads by up to 20%. >> >> Requirements for LTO enabling: >> - cmake >= 3.9 >> - linker >> - ld >= 2.31 (or latest 2.30) >> - gold >= 1.15 (binutils 2.30) >> - mac xcode >= 8 (earlier versions are not tested) >> This small patch required fixing bug in GNU ld linker. >> (link gcc.gnu.org/bugzilla/show_bug.cgi?id=84901) >> `ld` was not exporting symbols from dynamic list when LTO enabled. >> >> Closes #3117 >> --- >> CMakeLists.txt | 1 + >> cmake/lto.cmake | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> src/lib/msgpuck | 2 +- >> src/lib/small | 2 +- >> third_party/libyaml | 2 +- >> 5 files changed, 59 insertions(+), 3 deletions(-) >> create mode 100644 cmake/lto.cmake >> >> diff --git a/CMakeLists.txt b/CMakeLists.txt >> index d0cae0f01..432d2a6fb 100644 >> --- a/CMakeLists.txt >> +++ b/CMakeLists.txt >> @@ -63,6 +63,7 @@ include(cmake/pod2man.cmake) >> include(cmake/arch.cmake) >> include(cmake/os.cmake) >> include(cmake/compiler.cmake) >> +include(cmake/lto.cmake NO_POLICY_SCOPE) > Is it for propagating cmake_policy(SET CMP0069 NEW)? Please, comment it. Add commit at the beginning of `lto.cmake` file. I suppose it is the most suitable place. >> include(cmake/simd.cmake) >> include(cmake/atomic.cmake) >> include(cmake/profile.cmake) >> diff --git a/cmake/lto.cmake b/cmake/lto.cmake >> new file mode 100644 >> index 000000000..96129ebfd >> --- /dev/null >> +++ b/cmake/lto.cmake >> @@ -0,0 +1,55 @@ >> +## >> +## Manage LTO (Link-Time-Optimization) and IPO (Inter-Procedural-Optimization) >> +## >> + >> +# Tarantool uses both dynamic-list and lto link options, which works only >> +# since binutils: >> +# - 2.30 for linking with gold (gold version is 1.15) >> +# - last 2.30 or 2.31 in case of ld (bfd) >> + >> +if (NOT DEFINED TARANTOOL_LTO) >> + set(TARANTOOL_LTO FALSE) >> +endif() >> + >> +if(NOT CMAKE_VERSION VERSION_LESS 3.9) >> + cmake_policy(SET CMP0069 NEW) >> + include(CheckIPOSupported) >> + check_ipo_supported(RESULT CMAKE_IPO_AVAILABLE) >> +else() >> + set(CMAKE_IPO_AVAILABLE FALSE) >> +endif() >> + >> +# Ensure that default value is false >> +set(CMAKE_INTERPROCEDURAL_OPTIMIZATION FALSE) > Nit: separate this variable initialization from the `if` below (because > it related to two `if`s below). done >> +if (TARANTOOL_LTO AND CMAKE_IPO_AVAILABLE AND NOT TARGET_OS_DARWIN) >> + execute_process(COMMAND ld -v OUTPUT_VARIABLE linker_v) > Maybe it is better to ask for ld.bfd (instead of just ld) explicitly and > set -fuse-ld-bfd explicitly. Despite it is discouraged (afaik) to use > gold as the default system linker such environments are possible. I had > use one for couple of years, because of some problem with bfd. > > I think it would be more natural to check for ld.bfd existence as we do > for ld.gold. TODO >> + message(STATUS "LTO linker_v ${linker_v}") > A tab instead of a whitespace. fixed >> + >> + # e.g. GNU ld (GNU Binutils) 2.29 >> + string(REGEX MATCH "^GNU ld [^)]+[)] ([0-9.]+).*$" linker_valid ${linker_v}) >> + > On centos-7: > > $ ld.bfd -v > GNU ld version 2.27-10.el7 > > It seems it will not match, because of lack of the closing parenthesis. > > BTW, [)] is redundant, we can use \( (at least in PCRE). Fixed >> + if (NOT linker_valid) >> + message(FATAL_ERROR "Unsuported linker (ld expected)") > Typo: Unsuported -> Unsupported. fixed > >> + endif() >> + >> + set(linker_version ${CMAKE_MATCH_1}) >> + message(STATUS "Found linker ld VERSION ${linker_version}") >> + >> + if (NOT linker_version VERSION_LESS "2.31") >> + set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) >> + elseif(NOT linker_version VERSION_LESS "2.30") > Here you are check bfd version to make a decision about using gold. The > approach seems to be broken. > > Yep, I saw environments with ability to tweak PATH to obtain a specific > bfd/gold version (separately). discussed. Now the minimal supported binutils version is 2.31, and the ld is not changed implicitly. >> + # Use gold if LTO+dynamic-list is available in gold & not in ld >> + find_program(gold_available "ld.gold") >> + if (gold_available) >> + message(WARNING "Use gold linker (to enable LTO)") >> + SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fuse-ld=gold") >> + set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) >> + endif() >> + endif() >> +endif() >> + >> +if (TARANTOOL_LTO AND CMAKE_IPO_AVAILABLE AND TARGET_OS_DARWIN) >> + set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) >> +endif() >> + > I don't get why here we don't check anything? Are exporting symbol > problems actual here? Because it worked well on all mac environments that I found. >> +message(STATUS "LTO enabled ${CMAKE_INTERPROCEDURAL_OPTIMIZATION}") >> diff --git a/src/lib/msgpuck b/src/lib/msgpuck >> index 3b8f3e59b..1eb56a447 160000 >> --- a/src/lib/msgpuck >> +++ b/src/lib/msgpuck >> @@ -1 +1 @@ >> -Subproject commit 3b8f3e59b62d74f0198e01cbec0beb9c6a3082fb >> +Subproject commit 1eb56a447ea35f9c403dbc8aa98a25336303c1cb >> diff --git a/src/lib/small b/src/lib/small >> index 22d1bad18..0627a7f98 160000 >> --- a/src/lib/small >> +++ b/src/lib/small >> @@ -1 +1 @@ >> -Subproject commit 22d1bad1873edcb9b1383a273e70c4cf881d5349 >> +Subproject commit 0627a7f986ff91701bd741908818749fef5e2266 >> diff --git a/third_party/libyaml b/third_party/libyaml >> index 6bd4be1a7..e554afad0 160000 >> --- a/third_party/libyaml >> +++ b/third_party/libyaml >> @@ -1 +1 @@ >> -Subproject commit 6bd4be1a7751d6022a413864666880bef8a87c3c >> +Subproject commit e554afad015b2c7df76f9d5213d883e931246fad >> -- > We should not update submodules to non-master branches. Please, send > peliminary patches for submodules first. Ok. I will attach patches to the submodules to this tread. > >> 2.14.1 new diff: commit dfd9c70a525477299cc075b5f0e6acb6ed14e335 Author: AKhatskevich Date:   Thu Mar 22 11:37:06 2018 +0300     Add LTO support     This commit introduces LTO support.     Changes:       - update submodules (fix lto-related warnings)       - add `TARANTOOL_LTO` cmake option (by default it is `False`)     LTO speeds up cpu-intensive workloads by up to 20%.     Requirements for LTO enabling:       - cmake >= 3.9       - linker         - ld >= 2.31 (or latest 2.30)         - gold >= 1.15 (binutils 2.30)         - mac xcode >= 8 (earlier versions are not tested)     This small patch required fixing bug in GNU ld linker.     (link gcc.gnu.org/bugzilla/show_bug.cgi?id=84901)     `ld` was not exporting symbols from dynamic list when LTO enabled.     Closes #3117 diff --git a/CMakeLists.txt b/CMakeLists.txt index 439a2750a..08ca1bbfd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -63,6 +63,7 @@ include(cmake/pod2man.cmake)  include(cmake/arch.cmake)  include(cmake/os.cmake)  include(cmake/compiler.cmake) +include(cmake/lto.cmake NO_POLICY_SCOPE)  include(cmake/simd.cmake)  include(cmake/atomic.cmake)  include(cmake/profile.cmake) diff --git a/cmake/lto.cmake b/cmake/lto.cmake new file mode 100644 index 000000000..8e00ad302 --- /dev/null +++ b/cmake/lto.cmake @@ -0,0 +1,52 @@ +## +## Manage LTO (Link-Time-Optimization) and IPO +## (Inter-Procedural-Optimization) +## + +# Tarantool uses both dynamic-list and lto link options, which +# works only since binutils: +#  - 2.30 for linking with gold (gold version is 1.15) +#  - last 2.30 or 2.31 in case of ld (bfd) + +# This cmake module exports CMP0069 policy and should be included +# with NO_POLICY_SCOPE option. + +if (NOT DEFINED ENABLE_LTO) +    set(ENABLE_LTO FALSE) +endif() + +if(NOT CMAKE_VERSION VERSION_LESS 3.9) +    cmake_policy(SET CMP0069 NEW) +    include(CheckIPOSupported) +    check_ipo_supported(RESULT CMAKE_IPO_AVAILABLE) +else() +    set(CMAKE_IPO_AVAILABLE FALSE) +endif() + +# Ensure that default value is false +set(CMAKE_INTERPROCEDURAL_OPTIMIZATION FALSE) + +if (ENABLE_LTO AND CMAKE_IPO_AVAILABLE AND NOT TARGET_OS_DARWIN) +    execute_process(COMMAND ld.bfd -v OUTPUT_VARIABLE linker_v) +    message(STATUS "LTO linker_v ${linker_v}") + +    # e.g. GNU ld (GNU Binutils) 2.29 +    string(REGEX MATCH "^GNU (go)?ld [^0-9]+ ([2-3][.][0-9]+).*$" linker_valid ${linker_v}) + +    if (NOT linker_valid) +        message(FATAL_ERROR "Unsupported linker version format") +    endif() + +    set(linker_version ${CMAKE_MATCH_2}) +    message(STATUS "Found binutils VERSION ${linker_version}") + +    if (NOT linker_version VERSION_LESS "2.31") +        set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) +    endif() +endif() + +if (ENABLE_LTO AND CMAKE_IPO_AVAILABLE AND TARGET_OS_DARWIN) +    set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) +endif() + +message(STATUS "LTO enabled ${CMAKE_INTERPROCEDURAL_OPTIMIZATION}") diff --git a/src/lib/msgpuck b/src/lib/msgpuck index 3b8f3e59b..1eb56a447 160000 --- a/src/lib/msgpuck +++ b/src/lib/msgpuck @@ -1 +1 @@ -Subproject commit 3b8f3e59b62d74f0198e01cbec0beb9c6a3082fb +Subproject commit 1eb56a447ea35f9c403dbc8aa98a25336303c1cb diff --git a/src/lib/small b/src/lib/small index 22d1bad18..0627a7f98 160000 --- a/src/lib/small +++ b/src/lib/small @@ -1 +1 @@ -Subproject commit 22d1bad1873edcb9b1383a273e70c4cf881d5349 +Subproject commit 0627a7f986ff91701bd741908818749fef5e2266 diff --git a/third_party/libyaml b/third_party/libyaml index 6bd4be1a7..e554afad0 160000 --- a/third_party/libyaml +++ b/third_party/libyaml @@ -1 +1 @@ -Subproject commit 6bd4be1a7751d6022a413864666880bef8a87c3c +Subproject commit e554afad015b2c7df76f9d5213d883e931246fad