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

[Alex Khatskevich <avkhatskevich@tarantool.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 <avkhatskevich@tarantool.org>
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