[tarantool-patches] Re: [PATCH 2/3] Add LTO support

[Alexander Turenko <alexander.turenko@tarantool.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
> 
>