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 67D532C3B3 for ; Wed, 10 Oct 2018 10:29:53 -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 KR3uJvu6MkV0 for ; Wed, 10 Oct 2018 10:29:53 -0400 (EDT) Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 15F252C391 for ; Wed, 10 Oct 2018 10:29:52 -0400 (EDT) Date: Wed, 10 Oct 2018 17:29:48 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH 2/3] Add LTO support Message-ID: <20181010142948.nlwmr4bz77okripx@tkn_work_nb> References: <64060dd168d3977f90f06b0bd242cf7ecc256a07.1533726342.git.avkhatskevich@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <64060dd168d3977f90f06b0bd242cf7ecc256a07.1533726342.git.avkhatskevich@tarantool.org> 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: AKhatskevich Cc: georgy@tarantool.org, tarantool-patches@freelists.org 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. > - add `TARANTOOL_LTO` cmake option (by default it is `False`) It seems such options usually looks like ENABLE_XXX=[ON/OFF]. > > 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. > 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). > +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. > + message(STATUS "LTO linker_v ${linker_v}") A tab instead of a whitespace. > + > + # 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). > + if (NOT linker_valid) > + message(FATAL_ERROR "Unsuported linker (ld expected)") Typo: Unsuported -> Unsupported. > + 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). > + # 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? > +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. > 2.14.1 > >