[tarantool-patches] Re: [PATCH] Add LTO support

[Alexander Turenko]

Hi, Alex!

My comments are below.

WBR, Alexander Turenko.

On Tue, May 15, 2018 at 01:23:28PM +0300, AKhatskevich wrote:
 
> Changes:
>   - update submodules (fix lto-related warnings)
>   - add `TARANTOOL_LTO` cmake option (by default it is `True` only for
>     non-Debug builds)
> 

Don't remember any discussions re enabling this by default. Is it Kirill
or Kostya decision?

> 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)
> This small patch required fixing bug in GNU ld linker.

If you menting the bug here, so add the link.

> +# 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 (dbf)
> +#    (ld was fixed expecially for Tarantool,
> +#    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84901)
> +

dbf -> bfd

'Expecially' is 'especially'? I feel that as the boast in not
appropriate place.

> +    if (NOT linker_version VERSION_LESS "2.31")
> +	set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)

Strange indent. It seems that you mix tabs and spaces within that file.

> +    elseif(NOT linker_version VERSION_LESS "2.30")
> +      # 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)

Indent.