[Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Tarantool has LTO support, however while building luajit this opt. was
omitted. Patch adds necessary flag to turn it on.

Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.

Closes #3743
---
Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
 cmake/lto.cmake    |  3 ++-
 cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/cmake/lto.cmake b/cmake/lto.cmake
index 95ade75f4..9f29f3324 100644
--- a/cmake/lto.cmake
+++ b/cmake/lto.cmake
@@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
         if (linker_version VERSION_LESS "2.31")
             message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
         endif()
-    elseif(matched_gold)
+
+	elseif(matched_gold)
         set(linker_version ${CMAKE_MATCH_1})
         message(STATUS "Found ld.gold version: ${linker_version}")
 
diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 072db8269..da1b4926d 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -225,6 +225,32 @@ macro(luajit_build)
         set(luajit_xcflags ${luajit_xcflags} -D${def})
     endforeach()
 
+    # Add LTO option to luajit
+    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
+	    message("Setting LTO flags for building luajit")
+	# Clang opt to support LTO
+	if (CMAKE_COMPILER_IS_CLANG AND
+	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
+	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
+		set(luajit_cflags ${luajit_cflags} -flto=full)
+	    else()
+	        set(luajit_cflags ${luajit_cflags} -flto=full)
+	    endif()
+	if (NOT TARGET_OS_DARWIN)
+	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
+	   set (CMAKE_AR llvm-ar)
+	endif()
+	# GNU opts to support lto
+	else()
+	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
+	   #The same is for binutils prior 2.27
+	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
+	       AND NOT ${linker_version} VERSION_LESS 2.27)
+	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
+	   endif()
+	   set (CMAKE_AR gcc-ar)
+        endif()
+    endif()
     # Pass the same toolchain that is used for building of
     # tarantool itself, because tools from different toolchains
     # can be incompatible. A compiler and a linker are already set
-- 
2.20.1 (Apple Git-117)

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Cyrill Gorcunov]

On Thu, Mar 12, 2020 at 01:05:49PM +0300, Olga Arkhangelskaia wrote:
> Tarantool has LTO support, however while building luajit this opt. was
> omitted. Patch adds necessary flag to turn it on.
> 
> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> 
> Closes #3743
> ---
> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci

Been testing it on Fedora29

Tested-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Igor Munkin]

Olya,

Thanks for the patch! I left several comments below, please consider
them.

On 12.03.20, Olga Arkhangelskaia wrote:
> Tarantool has LTO support, however while building luajit this opt. was
> omitted. Patch adds necessary flag to turn it on.
> 
> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> 
> Closes #3743
> ---
> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>  cmake/lto.cmake    |  3 ++-
>  cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
> index 95ade75f4..9f29f3324 100644
> --- a/cmake/lto.cmake
> +++ b/cmake/lto.cmake
> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>          if (linker_version VERSION_LESS "2.31")
>              message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>          endif()
> -    elseif(matched_gold)
> +
> +	elseif(matched_gold)

Typo: there is a mess with whitespace above.

However this chunk provides no changes except whitespace and newline.
Please drop it.

>          set(linker_version ${CMAKE_MATCH_1})
>          message(STATUS "Found ld.gold version: ${linker_version}")
>  
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 072db8269..da1b4926d 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -225,6 +225,32 @@ macro(luajit_build)
>          set(luajit_xcflags ${luajit_xcflags} -D${def})
>      endforeach()
>  
> +    # Add LTO option to luajit
> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> +	    message("Setting LTO flags for building luajit")
> +	# Clang opt to support LTO
> +	if (CMAKE_COMPILER_IS_CLANG AND
> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)

So, if clang toolchain is used but version is less than 3.4, then gcc
toolchain settings are used. It's either totally wrong or definitely
need to be clarified with a corresponding comment.

Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
CMake module. So additional check here looks to be excess or also need
to be clarified.

> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +		set(luajit_cflags ${luajit_cflags} -flto=full)
> +	    else()
> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
> +	    endif()

I see no difference in the branches above.

> +	if (NOT TARGET_OS_DARWIN)
> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
> +	   set (CMAKE_AR llvm-ar)

What ar is used on systems different from Darwin based ones when clang
toolchain is used?

> +	endif()
> +	# GNU opts to support lto
> +	else()
> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
> +	   #The same is for binutils prior 2.27
> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> +	   endif()
> +	   set (CMAKE_AR gcc-ar)

Could you please clarify why ar is set explicitly here?

> +        endif()
> +    endif()

Typo: there is a mess with indent above. Unfortunately I failed to find
our CMake style guide, so please adjust your changes considering the
code nearby.

>      # Pass the same toolchain that is used for building of
>      # tarantool itself, because tools from different toolchains
>      # can be incompatible. A compiler and a linker are already set
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]

Hello Igor,

thanks for the review!

14.04.2020 12:18, Igor Munkin пишет:
> Olya,
>
> Thanks for the patch! I left several comments below, please consider
> them.
>
> On 12.03.20, Olga Arkhangelskaia wrote:
>> Tarantool has LTO support, however while building luajit this opt. was
>> omitted. Patch adds necessary flag to turn it on.
>>
>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
>>
>> Closes #3743
>> ---
>> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>>   cmake/lto.cmake    |  3 ++-
>>   cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>> index 95ade75f4..9f29f3324 100644
>> --- a/cmake/lto.cmake
>> +++ b/cmake/lto.cmake
>> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>>           if (linker_version VERSION_LESS "2.31")
>>               message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>>           endif()
>> -    elseif(matched_gold)
>> +
>> +	elseif(matched_gold)
> Typo: there is a mess with whitespace above.
>
> However this chunk provides no changes except whitespace and newline.
> Please drop it.
Will fix1
>
>>           set(linker_version ${CMAKE_MATCH_1})
>>           message(STATUS "Found ld.gold version: ${linker_version}")
>>   
>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>> index 072db8269..da1b4926d 100644
>> --- a/cmake/luajit.cmake
>> +++ b/cmake/luajit.cmake
>> @@ -225,6 +225,32 @@ macro(luajit_build)
>>           set(luajit_xcflags ${luajit_xcflags} -D${def})
>>       endforeach()
>>   
>> +    # Add LTO option to luajit
>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>> +	    message("Setting LTO flags for building luajit")
>> +	# Clang opt to support LTO
>> +	if (CMAKE_COMPILER_IS_CLANG AND
>> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
> So, if clang toolchain is used but version is less than 3.4, then gcc
> toolchain settings are used. It's either totally wrong or definitely
> need to be clarified with a corresponding comment.
>
> Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
> CMake module. So additional check here looks to be excess or also need
> to be clarified.

I have a mistake here (Thin LTO) - will wix it

If version is above than 3.9 we can use -flto=thin

>
>> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>> +		set(luajit_cflags ${luajit_cflags} -flto=full)
>> +	    else()
>> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
>> +	    endif()
> I see no difference in the branches above.
>
>> +	if (NOT TARGET_OS_DARWIN)
>> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>> +	   set (CMAKE_AR llvm-ar)
> What ar is used on systems different from Darwin based ones when clang
> toolchain is used?


gcc-ar/ llvm-ar  is wrapper on ar that simply passes —plugin option. We 
need it for LTO.

( See https://bugzilla.redhat.com/show_bug.cgi?id=1678826

https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html) 
<https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html>


>
>> +	endif()
>> +	# GNU opts to support lto
>> +	else()
>> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>> +	   #The same is for binutils prior 2.27
>> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
>> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
>> +	   endif()
>> +	   set (CMAKE_AR gcc-ar)
> Could you please clarify why ar is set explicitly here?

>
>> +        endif()
>> +    endif()
> Typo: there is a mess with indent above. Unfortunately I failed to find
> our CMake style guide, so please adjust your changes considering the
> code nearby.
Will fix
>
>>       # Pass the same toolchain that is used for building of
>>       # tarantool itself, because tools from different toolchains
>>       # can be incompatible. A compiler and a linker are already set
>> -- 
>> 2.20.1 (Apple Git-117)
>>

[-- Attachment #2: Type: text/html, Size: 6221 bytes --]

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Hi once again!

I have fixed your comments and pushed once again. See below.

14.04.2020 12:18, Igor Munkin пишет:
> Olya,
>
> Thanks for the patch! I left several comments below, please consider
> them.
>
> On 12.03.20, Olga Arkhangelskaia wrote:
>> Tarantool has LTO support, however while building luajit this opt. was
>> omitted. Patch adds necessary flag to turn it on.
>>
>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
>>
>> Closes #3743
>> ---
>> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>>   cmake/lto.cmake    |  3 ++-
>>   cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>> index 95ade75f4..9f29f3324 100644
>> --- a/cmake/lto.cmake
>> +++ b/cmake/lto.cmake
>> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>>           if (linker_version VERSION_LESS "2.31")
>>               message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>>           endif()
>> -    elseif(matched_gold)
>> +
>> +	elseif(matched_gold)
> Typo: there is a mess with whitespace above.
>
> However this chunk provides no changes except whitespace and newline.
> Please drop it.
>
Tarantool has LTO support, however while building luajit this opt. was
omitted. Patch adds necessary flag to turn it on.

Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.

Closes #3743
---
  cmake/luajit.cmake | 23 +++++++++++++++++++++++
  1 file changed, 23 insertions(+)


>>           set(linker_version ${CMAKE_MATCH_1})
>>           message(STATUS "Found ld.gold version: ${linker_version}")
>>   
>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>> index 072db8269..da1b4926d 100644
>> --- a/cmake/luajit.cmake
>> +++ b/cmake/luajit.cmake
>> @@ -225,6 +225,32 @@ macro(luajit_build)
>>           set(luajit_xcflags ${luajit_xcflags} -D${def})
>>       endforeach()
>>   
>> +    # Add LTO option to luajit
>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>> +	    message("Setting LTO flags for building luajit")
>> +	# Clang opt to support LTO
>> +	if (CMAKE_COMPILER_IS_CLANG AND
>> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
> So, if clang toolchain is used but version is less than 3.4, then gcc
> toolchain settings are used. It's either totally wrong or definitely
> need to be clarified with a corresponding comment.
>
> Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
> CMake module. So additional check here looks to be excess or also need
> to be clarified.
>
>> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>> +		set(luajit_cflags ${luajit_cflags} -flto=full)
>> +	    else()
>> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
>> +	    endif()
> I see no difference in the branches above.
>
>> +	if (NOT TARGET_OS_DARWIN)
>> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>> +	   set (CMAKE_AR llvm-ar)
> What ar is used on systems different from Darwin based ones when clang
> toolchain is used?
>
>> +	endif()
>> +	# GNU opts to support lto
>> +	else()
>> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>> +	   #The same is for binutils prior 2.27
>> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
>> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
>> +	   endif()
>> +	   set (CMAKE_AR gcc-ar)
> Could you please clarify why ar is set explicitly here?
>
>> +        endif()
>> +    endif()
+    # Add LTO option to luajit
+    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
+           message("Setting LTO flags for building luajit")
+       # Clang opt to support LTO
+       if (CMAKE_COMPILER_IS_CLANG)
+           if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
+               set(luajit_cflags ${luajit_cflags} -flto=full)
+ else()
+               set(luajit_cflags ${luajit_cflags} -flto=thin)
+ endif()
+          # Depending of compiler we need to set appropriate 
llvm/gcc-ar lib
+          set (CMAKE_AR llvm-ar)
+       # GNU opts to support lto
+ else()
+          #Due to some problems (bugs, slow work, etc) we support LTO 
only for 5.0+
+          #The same is for binutils prior 2.27
+          if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
+              AND NOT ${linker_version} VERSION_LESS 2.27)
+              set(luajit_cflags ${luajit_cflags} -flto 
-fuse-linker-plugin -fno-fat-lto-objects)
+ endif()
+          set (CMAKE_AR gcc-ar)
+ endif()
+    endif()
> Typo: there is a mess with indent above. Unfortunately I failed to find
> our CMake style guide, so please adjust your changes considering the
> code nearby.
>
>>       # Pass the same toolchain that is used for building of
>>       # tarantool itself, because tools from different toolchains
>>       # can be incompatible. A compiler and a linker are already set
>> -- 
>> 2.20.1 (Apple Git-117)
>>

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Fixed tabs:

  +            # Depending of compiler we need to set appropriate 
llvm/gcc-ar lib
  +            # llvm/gcc -ar is just a wrapper over ar to pass 
--plugin= option,
  +            # so that ar can understand LTO objects.

14.04.2020 12:18, Igor Munkin пишет:
> Olya,
>
> Thanks for the patch! I left several comments below, please consider
> them.
>
> On 12.03.20, Olga Arkhangelskaia wrote:
>> Tarantool has LTO support, however while building luajit this opt. was
>> omitted. Patch adds necessary flag to turn it on.
>>
>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
>>
>> Closes #3743
>> ---
>> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>>   cmake/lto.cmake    |  3 ++-
>>   cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>> index 95ade75f4..9f29f3324 100644
>> --- a/cmake/lto.cmake
>> +++ b/cmake/lto.cmake
>> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>>           if (linker_version VERSION_LESS "2.31")
>>               message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>>           endif()
>> -    elseif(matched_gold)
>> +
>> +	elseif(matched_gold)
> Typo: there is a mess with whitespace above.
>
> However this chunk provides no changes except whitespace and newline.
> Please drop it.
>
>>           set(linker_version ${CMAKE_MATCH_1})
>>           message(STATUS "Found ld.gold version: ${linker_version}")
>>   
>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>> index 072db8269..da1b4926d 100644
>> --- a/cmake/luajit.cmake
>> +++ b/cmake/luajit.cmake
>> @@ -225,6 +225,32 @@ macro(luajit_build)
>>           set(luajit_xcflags ${luajit_xcflags} -D${def})
>>       endforeach()
>>   
>> +    # Add LTO option to luajit
>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>> +	    message("Setting LTO flags for building luajit")
>> +	# Clang opt to support LTO
>> +	if (CMAKE_COMPILER_IS_CLANG AND
>> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
> So, if clang toolchain is used but version is less than 3.4, then gcc
> toolchain settings are used. It's either totally wrong or definitely
> need to be clarified with a corresponding comment.
>
> Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
> CMake module. So additional check here looks to be excess or also need
> to be clarified.
>
>> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>> +		set(luajit_cflags ${luajit_cflags} -flto=full)
>> +	    else()
>> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
>> +	    endif()
> I see no difference in the branches above.
>
>> +	if (NOT TARGET_OS_DARWIN)
>> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>> +	   set (CMAKE_AR llvm-ar)
> What ar is used on systems different from Darwin based ones when clang
> toolchain is used?
>
>> +	endif()
>> +	# GNU opts to support lto
>> +	else()
>> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>> +	   #The same is for binutils prior 2.27
>> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
>> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
>> +	   endif()
>> +	   set (CMAKE_AR gcc-ar)
> Could you please clarify why ar is set explicitly here?
>
>> +        endif()
>> +    endif()
> Typo: there is a mess with indent above. Unfortunately I failed to find
> our CMake style guide, so please adjust your changes considering the
> code nearby.
>
>>       # Pass the same toolchain that is used for building of
>>       # tarantool itself, because tools from different toolchains
>>       # can be incompatible. A compiler and a linker are already set
>> -- 
>> 2.20.1 (Apple Git-117)
>>

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Igor Munkin]

Olya,

Thanks for the fixes! There are still a couple nits below.

On 14.04.20, Olga Arkhangelskaia wrote:
> Hi once again!
> 
> I have fixed your comments and pushed once again. See below.
> 
> 14.04.2020 12:18, Igor Munkin пишет:
> > Olya,
> >
> > Thanks for the patch! I left several comments below, please consider
> > them.
> >
> > On 12.03.20, Olga Arkhangelskaia wrote:
> >> Tarantool has LTO support, however while building luajit this opt. was
> >> omitted. Patch adds necessary flag to turn it on.
> >>
> >> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> >>
> >> Closes #3743
> >> ---
> >> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
> >>   cmake/lto.cmake    |  3 ++-
> >>   cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
> >>   2 files changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
> >> index 95ade75f4..9f29f3324 100644
> >> --- a/cmake/lto.cmake
> >> +++ b/cmake/lto.cmake
> >> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
> >>           if (linker_version VERSION_LESS "2.31")
> >>               message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
> >>           endif()
> >> -    elseif(matched_gold)
> >> +
> >> +	elseif(matched_gold)
> > Typo: there is a mess with whitespace above.
> >
> > However this chunk provides no changes except whitespace and newline.
> > Please drop it.
> >
> Tarantool has LTO support, however while building luajit this opt. was
> omitted. Patch adds necessary flag to turn it on.
> 
> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> 
> Closes #3743
> ---
>   cmake/luajit.cmake | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> 
> >>           set(linker_version ${CMAKE_MATCH_1})
> >>           message(STATUS "Found ld.gold version: ${linker_version}")
> >>   
> >> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> >> index 072db8269..da1b4926d 100644
> >> --- a/cmake/luajit.cmake
> >> +++ b/cmake/luajit.cmake
> >> @@ -225,6 +225,32 @@ macro(luajit_build)
> >>           set(luajit_xcflags ${luajit_xcflags} -D${def})
> >>       endforeach()
> >>   
> >> +    # Add LTO option to luajit
> >> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> >> +	    message("Setting LTO flags for building luajit")
> >> +	# Clang opt to support LTO
> >> +	if (CMAKE_COMPILER_IS_CLANG AND
> >> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
> > So, if clang toolchain is used but version is less than 3.4, then gcc
> > toolchain settings are used. It's either totally wrong or definitely
> > need to be clarified with a corresponding comment.
> >
> > Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
> > CMake module. So additional check here looks to be excess or also need
> > to be clarified.
> >
> >> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> >> +		set(luajit_cflags ${luajit_cflags} -flto=full)
> >> +	    else()
> >> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
> >> +	    endif()
> > I see no difference in the branches above.
> >
> >> +	if (NOT TARGET_OS_DARWIN)
> >> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
> >> +	   set (CMAKE_AR llvm-ar)
> > What ar is used on systems different from Darwin based ones when clang
> > toolchain is used?
> >
> >> +	endif()
> >> +	# GNU opts to support lto
> >> +	else()
> >> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
> >> +	   #The same is for binutils prior 2.27
> >> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> >> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
> >> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> >> +	   endif()
> >> +	   set (CMAKE_AR gcc-ar)
> > Could you please clarify why ar is set explicitly here?
> >
> >> +        endif()
> >> +    endif()
> +    # Add LTO option to luajit
> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> +           message("Setting LTO flags for building luajit")
> +       # Clang opt to support LTO
> +       if (CMAKE_COMPILER_IS_CLANG)
> +           if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +               set(luajit_cflags ${luajit_cflags} -flto=full)
> + else()
> +               set(luajit_cflags ${luajit_cflags} -flto=thin)
> + endif()

Moved our discussion below:

================================================================================

> > I see no difference in the branches above.
> 
> I have a mistake here (Thin LTO) - will wix it
> 
> If version is above than 3.9 we can use -flto=thin

What is the difference between "full" and "thin" values? Please drop a
few words regarding it.

================================================================================

> +          # Depending of compiler we need to set appropriate 
> llvm/gcc-ar lib
> +          set (CMAKE_AR llvm-ar)

Moved our discussion below:

================================================================================

> > What ar is used on systems different from Darwin based ones when clang
> > toolchain is used?
> 
> 
> gcc-ar/ llvm-ar  is wrapper on ar that simply passes —plugin option. We 
> need it for LTO.
> 
> ( See https://bugzilla.redhat.com/show_bug.cgi?id=1678826
> 
> https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html) 
> <https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html>

It would be nice to leave a comment regarding it.

================================================================================

> +       # GNU opts to support lto

Minor: Please move this comment under <else()> statement as the
corresponding clang-related comment.

> + else()
> +          #Due to some problems (bugs, slow work, etc) we support LTO 
> only for 5.0+
> +          #The same is for binutils prior 2.27

Typo: Please separate # from the comment with a space.

> +          if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> +              AND NOT ${linker_version} VERSION_LESS 2.27)

Are there any versions supporting LTO/IPO less than gcc-5.0 and ld-2.27?
If yes, please mention why they don't fit our purposes. If not, I see no
rationale for this condition.

> +              set(luajit_cflags ${luajit_cflags} -flto 
> -fuse-linker-plugin -fno-fat-lto-objects)
> + endif()
> +          set (CMAKE_AR gcc-ar)

Minor: there are some <set> commands with a whitespace prior to
parentheses and some without. Please adjust those you introduced within
your changes to a single style.

> + endif()
> +    endif()
> > Typo: there is a mess with indent above. Unfortunately I failed to find
> > our CMake style guide, so please adjust your changes considering the
> > code nearby.
> >
> >>       # Pass the same toolchain that is used for building of
> >>       # tarantool itself, because tools from different toolchains
> >>       # can be incompatible. A compiler and a linker are already set
> >> -- 
> >> 2.20.1 (Apple Git-117)
> >>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Hi Igor!

See diff below:

14.04.2020 20:00, Igor Munkin пишет:
> Olya,
>
> Thanks for the fixes! There are still a couple nits below.
>
> On 14.04.20, Olga Arkhangelskaia wrote:
>> Hi once again!
>>
>> I have fixed your comments and pushed once again. See below.
>>
>> 14.04.2020 12:18, Igor Munkin пишет:
>>> Olya,
>>>
>>> Thanks for the patch! I left several comments below, please consider
>>> them.
>>>
>>> On 12.03.20, Olga Arkhangelskaia wrote:
>>>> Tarantool has LTO support, however while building luajit this opt. was
>>>> omitted. Patch adds necessary flag to turn it on.
>>>>
>>>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
>>>>
>>>> Closes #3743
>>>> ---
>>>> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>>>>    cmake/lto.cmake    |  3 ++-
>>>>    cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>>>>    2 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>>>> index 95ade75f4..9f29f3324 100644
>>>> --- a/cmake/lto.cmake
>>>> +++ b/cmake/lto.cmake
>>>> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>>>>            if (linker_version VERSION_LESS "2.31")
>>>>                message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>>>>            endif()
>>>> -    elseif(matched_gold)
>>>> +
>>>> +	elseif(matched_gold)
>>> Typo: there is a mess with whitespace above.
>>>
>>> However this chunk provides no changes except whitespace and newline.
>>> Please drop it.
>>>
>> Tarantool has LTO support, however while building luajit this opt. was
>> omitted. Patch adds necessary flag to turn it on.
>>
>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
>>
>> Closes #3743
>> ---
>>    cmake/luajit.cmake | 23 +++++++++++++++++++++++
>>    1 file changed, 23 insertions(+)
>>
>>
>>>>            set(linker_version ${CMAKE_MATCH_1})
>>>>            message(STATUS "Found ld.gold version: ${linker_version}")
>>>>    
>>>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>>>> index 072db8269..da1b4926d 100644
>>>> --- a/cmake/luajit.cmake
>>>> +++ b/cmake/luajit.cmake
>>>> @@ -225,6 +225,32 @@ macro(luajit_build)
>>>>            set(luajit_xcflags ${luajit_xcflags} -D${def})
>>>>        endforeach()
>>>>    
>>>> +    # Add LTO option to luajit
>>>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>>>> +	    message("Setting LTO flags for building luajit")
>>>> +	# Clang opt to support LTO
>>>> +	if (CMAKE_COMPILER_IS_CLANG AND
>>>> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
>>> So, if clang toolchain is used but version is less than 3.4, then gcc
>>> toolchain settings are used. It's either totally wrong or definitely
>>> need to be clarified with a corresponding comment.
>>>
>>> Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
>>> CMake module. So additional check here looks to be excess or also need
>>> to be clarified.
>>>
>>>> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>>>> +		set(luajit_cflags ${luajit_cflags} -flto=full)
>>>> +	    else()
>>>> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
>>>> +	    endif()
>>> I see no difference in the branches above.
>>>
>>>> +	if (NOT TARGET_OS_DARWIN)
>>>> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>>>> +	   set (CMAKE_AR llvm-ar)
>>> What ar is used on systems different from Darwin based ones when clang
>>> toolchain is used?
>>>
>>>> +	endif()
>>>> +	# GNU opts to support lto
>>>> +	else()
>>>> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>>>> +	   #The same is for binutils prior 2.27
>>>> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>>>> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
>>>> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
>>>> +	   endif()
>>>> +	   set (CMAKE_AR gcc-ar)
>>> Could you please clarify why ar is set explicitly here?
>>>
>>>> +        endif()
>>>> +    endif()
>> +    # Add LTO option to luajit
>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>> +           message("Setting LTO flags for building luajit")
>> +       # Clang opt to support LTO
>> +       if (CMAKE_COMPILER_IS_CLANG)
>> +           if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>> +               set(luajit_cflags ${luajit_cflags} -flto=full)
>> + else()
>> +               set(luajit_cflags ${luajit_cflags} -flto=thin)
>> + endif()
> Moved our discussion below:
>
> ================================================================================
>
>>> I see no difference in the branches above.
>> I have a mistake here (Thin LTO) - will wix it
>>
>> If version is above than 3.9 we can use -flto=thin
> What is the difference between "full" and "thin" values? Please drop a
> few words regarding it.
>
> ================================================================================
>
>> +          # Depending of compiler we need to set appropriate
>> llvm/gcc-ar lib
>> +          set (CMAKE_AR llvm-ar)
> Moved our discussion below:
>
> ================================================================================
>
>>> What ar is used on systems different from Darwin based ones when clang
>>> toolchain is used?
>>
>> gcc-ar/ llvm-ar  is wrapper on ar that simply passes —plugin option. We
>> need it for LTO.
>>
>> ( See https://bugzilla.redhat.com/show_bug.cgi?id=1678826
>>
>> https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html)
>> <https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html>
> It would be nice to leave a comment regarding it.
>
> ================================================================================
>
>> +       # GNU opts to support lto
> Minor: Please move this comment under <else()> statement as the
> corresponding clang-related comment.
>
>> + else()
>> +          #Due to some problems (bugs, slow work, etc) we support LTO
>> only for 5.0+
>> +          #The same is for binutils prior 2.27
> Typo: Please separate # from the comment with a space.
>
>> +          if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>> +              AND NOT ${linker_version} VERSION_LESS 2.27)
> Are there any versions supporting LTO/IPO less than gcc-5.0 and ld-2.27?
> If yes, please mention why they don't fit our purposes. If not, I see no
> rationale for this condition.
>
>> +              set(luajit_cflags ${luajit_cflags} -flto
>> -fuse-linker-plugin -fno-fat-lto-objects)
>> + endif()
>> +          set (CMAKE_AR gcc-ar)
> Minor: there are some <set> commands with a whitespace prior to
> parentheses and some without. Please adjust those you introduced within
> your changes to a single style.
>
>> + endif()
>> +    endif()
>>> Typo: there is a mess with indent above. Unfortunately I failed to find
>>> our CMake style guide, so please adjust your changes considering the
>>> code nearby.
>>>
>>>>        # Pass the same toolchain that is used for building of
>>>>        # tarantool itself, because tools from different toolchains
>>>>        # can be incompatible. A compiler and a linker are already set
>>>> -- 
>>>> 2.20.1 (Apple Git-117)
>>>>

            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)

-                set(luajit_cflags ${luajit_cflags} -flto=full)
+                set (luajit_cflags ${luajit_cflags} -flto=full)
              else()
-                set(luajit_cflags ${luajit_cflags} -flto=thin)
+               # ThinLTO that is both scalable and incremental
+               # due to parallel IPO, available since 3.9 and above.
+               # See 
http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
+               set (luajit_cflags ${luajit_cflags} -flto=thin)
              endif()
              # Depending of compiler we need to set appropriate 
llvm/gcc-ar lib
              # llvm/gcc -ar is just a wrapper over ar to pass --plugin= 
option,
              # so that ar can understand LTO objects.
-            set (CMAKE_AR llvm-ar)
-        # GNU opts to support lto
+            # See 
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
+           set (CMAKE_AR llvm-ar)
          else()
-        # Due to some problems (bugs, slow work, etc) we support LTO 
only for 5.0+
-        # The same is for binutils prior 2.27
+       # GNU opts to support lto
+               # Due to some problems (bugs, slow work, etc) we support 
LTO only for 5.0+
+               # The same is for binutils prior 2.27
+               # See https://patchwork.kernel.org/patch/10078207/
              if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
                  AND NOT ${linker_version} VERSION_LESS 2.27)
-                set(luajit_cflags ${luajit_cflags} -flto 
-fuse-linker-plugin -fno-fat-lto-objects)
+                set (luajit_cflags ${luajit_cflags} -flto 
-fuse-linker-plugin -fno-fat-lto-objects)
              endif()
+           # The same as for llvm-ar
+           # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Igor Munkin]

Olya,

Thanks for the fixes!

On 15.04.20, Olga Arkhangelskaia wrote:
> Hi Igor!
> 
> See diff below:
> 
> 14.04.2020 20:00, Igor Munkin пишет:

<snipped>

Sorry, but I failed to read the diff you placed at the end. Since I'm
using the same editor you do (vim, right?) let me share with you the
recipe I use for that purposes:

1. Save the diff you're going to paste to a temporary file.
2. Move the cursor to the place you're going to place the diff.
3. Run the following command:
| :r <diff/file/name>

I placed the diff from the upstream branch below.

The wording now is definitely the way much better, thanks! However,
please consider several nits I left inline.

Unfortunately, there is still a mess with a whitespace. Please fix it.

================================================================================

From: Olga Arkhangelskaia <arkholga@tarantool.org>
Subject: [PATCH] cmake: add LTO support for building luajit

Tarantool has LTO support, however while building luajit this opt. was
omitted. Patch adds necessary flag to turn it on.

Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
errors and slow work.

Closes #3743
---
 cmake/luajit.cmake | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 072db8269..a0d2768b2 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -225,6 +225,38 @@ macro(luajit_build)
         set(luajit_xcflags ${luajit_xcflags} -D${def})
     endforeach()
 
+    # Add LTO option to luajit
+    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
+        message("Setting LTO flags for building luajit")
+        # Clang opt to support LTO
+        if (CMAKE_COMPILER_IS_CLANG)
+            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
+                set (luajit_cflags ${luajit_cflags} -flto=full)
+            else()
+		# ThinLTO that is both scalable and incremental
+		# due to parallel IPO, available since 3.9 and above.
+		# See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html

Side note: great and clear wording, thanks!

+		set (luajit_cflags ${luajit_cflags} -flto=thin)
+            endif()
+            # Depending of compiler we need to set appropriate llvm/gcc-ar lib

Typo: s/Depending of/Depending on/.

+            # llvm/gcc -ar is just a wrapper over ar to pass --plugin= option,

I guess we can omit all gcc mentions within this comment and move all
gcc-related specifics below.

+            # so that ar can understand LTO objects.
+            # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html 

Typo: trailing whitespace above.

+	    set (CMAKE_AR llvm-ar)
+        else()
+	# GNU opts to support lto
+		# Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+

Typo: Line exceeds 80 symbols (furthermore, AFAIR comments are limited
with 66 symbols, but I don't know whether this limit relates to CMake
sources).

+		# The same is for binutils prior 2.27

Typo: s/prior/prior to/.

+		# See https://patchwork.kernel.org/patch/10078207/

Minor: It's a well structured long-read. I propose to point rationale
the following way (leaving other lines as is):
| # See comments in scripts/Makefile.lto in scope of
| # the patch: https://patchwork.kernel.org/patch/10078207/

+            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
+                AND NOT ${linker_version} VERSION_LESS 2.27)
+                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
+            endif()
+	    # The same as for llvm-ar
+	    # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826

GGC specifics are better described here[1]. So the comment can be
reworded the following way:
| # gcc-ar is just a wrapper over ar to pass --plugin option
| # so ar can understand LTO objects. For further info see
| # -flto and -ffat-lto-objects options descriptions:
| # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

Typo: an excess comma.

+            set (CMAKE_AR gcc-ar)
+        endif()
+    endif()
     # Pass the same toolchain that is used for building of
     # tarantool itself, because tools from different toolchains
     # can be incompatible. A compiler and a linker are already set
-- 
2.25.0

================================================================================

I see the tests are failed. The following jobs failed due to flaky
tests, so I just restarted them:
* release_clang[2]
* release_asan_clang8[3]
* osx_13_release[4]

However, there are some builds failed due to the changes:
* osx_14_release_lto[5]:
|  AR        libluajit.a
|  make[4]: llvm-ar: No such file or directory

There are also failed jobs that are removed in bleeding master (Ubuntu
18.10 and Ubuntu 19.04 jobs are removed considering distro version EOL).
Please rebase you branch on the master branch to fix this issue.

> 

[1]: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
[1]: https://gitlab.com/tarantool/tarantool/-/jobs/515804361
[2]: https://gitlab.com/tarantool/tarantool/-/jobs/515812948
[3]: https://gitlab.com/tarantool/tarantool/-/jobs/515827993
[3]: https://gitlab.com/tarantool/tarantool/-/jobs/512029069#L1912

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Hi Igor!

It seems that I eventually had fixed white-space mess.

While testing everything once again I came across reason why I separated 
case with Darwin and

llvm-ar. Fixed and added a comment.

Diffs are below:

P.S.

Please, let me know whether new diff is better.

17.04.2020 14:47, Igor Munkin пишет:
> Olya,
>
> Thanks for the fixes!
>
> On 15.04.20, Olga Arkhangelskaia wrote:
>> Hi Igor!
>>
>> See diff below:
>>
>> 14.04.2020 20:00, Igor Munkin пишет:
> <snipped>
>
> Sorry, but I failed to read the diff you placed at the end. Since I'm
> using the same editor you do (vim, right?) let me share with you the
> recipe I use for that purposes:
>
> 1. Save the diff you're going to paste to a temporary file.
> 2. Move the cursor to the place you're going to place the diff.
> 3. Run the following command:
> | :r <diff/file/name>
>
> I placed the diff from the upstream branch below.
>
> The wording now is definitely the way much better, thanks! However,
> please consider several nits I left inline.
>
> Unfortunately, there is still a mess with a whitespace. Please fix it.
>
> ================================================================================
>
> From: Olga Arkhangelskaia <arkholga@tarantool.org>
> Subject: [PATCH] cmake: add LTO support for building luajit
>
> Tarantool has LTO support, however while building luajit this opt. was
> omitted. Patch adds necessary flag to turn it on.
>
> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
> errors and slow work.
>
> Closes #3743
> ---
>   cmake/luajit.cmake | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 072db8269..a0d2768b2 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -225,6 +225,38 @@ macro(luajit_build)
>           set(luajit_xcflags ${luajit_xcflags} -D${def})
>       endforeach()
>   
> +    # Add LTO option to luajit
> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> +        message("Setting LTO flags for building luajit")
> +        # Clang opt to support LTO
> +        if (CMAKE_COMPILER_IS_CLANG)
> +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +                set (luajit_cflags ${luajit_cflags} -flto=full)
> +            else()
> +		# ThinLTO that is both scalable and incremental
> +		# due to parallel IPO, available since 3.9 and above.
> +		# See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
>
> Side note: great and clear wording, thanks!
>
> +		set (luajit_cflags ${luajit_cflags} -flto=thin)
> +            endif()
> +            # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>
> Typo: s/Depending of/Depending on/.
>
> +            # llvm/gcc -ar is just a wrapper over ar to pass --plugin= option,
>
> I guess we can omit all gcc mentions within this comment and move all
> gcc-related specifics below.
>
> +            # so that ar can understand LTO objects.
> +            # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
>
> Typo: trailing whitespace above.
>
> +	    set (CMAKE_AR llvm-ar)
> +        else()
> +	# GNU opts to support lto
> +		# Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>
> Typo: Line exceeds 80 symbols (furthermore, AFAIR comments are limited
> with 66 symbols, but I don't know whether this limit relates to CMake
> sources).
>
> +		# The same is for binutils prior 2.27
>
> Typo: s/prior/prior to/.
>
> +		# See https://patchwork.kernel.org/patch/10078207/
>
> Minor: It's a well structured long-read. I propose to point rationale
> the following way (leaving other lines as is):
> | # See comments in scripts/Makefile.lto in scope of
> | # the patch: https://patchwork.kernel.org/patch/10078207/
>
> +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> +                AND NOT ${linker_version} VERSION_LESS 2.27)
> +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> +            endif()
> +	    # The same as for llvm-ar
> +	    # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826
>
> GGC specifics are better described here[1]. So the comment can be
> reworded the following way:
> | # gcc-ar is just a wrapper over ar to pass --plugin option
> | # so ar can understand LTO objects. For further info see
> | # -flto and -ffat-lto-objects options descriptions:
> | # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
>
> Typo: an excess comma.
>
> +            set (CMAKE_AR gcc-ar)
> +        endif()
> +    endif()
>       # Pass the same toolchain that is used for building of
>       # tarantool itself, because tools from different toolchains
>       # can be incompatible. A compiler and a linker are already set

@@ -233,27 +233,29 @@ macro(luajit_build)
   6              if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
   7                  set (luajit_cflags ${luajit_cflags} -flto=full)
   8 else()
   9 -`......`.......# ThinLTO that is both scalable and incremental
  10 -`......`.......# due to parallel IPO, available since 3.9 and above.
  11 -`......`.......# See 
http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
  12 -`......`.......set (luajit_cflags ${luajit_cflags} -flto=thin)
  13 +                # ThinLTO that is both scalable and incremental
  14 +                # due to parallel IPO, available since 3.9 and above.
  15 +                # See 
http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
  16 +                set (luajit_cflags ${luajit_cflags} -flto=thin)
  17 endif()
  18 -            # Depending of compiler we need to set appropriate 
llvm/gcc-ar lib
  19 -            # llvm/gcc -ar is just a wrapper over ar to pass 
--plugin= option,
  20 -            # so that ar can understand LTO objects.
  21 -            # See 
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html~
  22 -`......    set (CMAKE_AR llvm-ar)
  23 +                # Since Darwin uses DYLD_LIBRARY_PATH instead of 
llvm-ar
  24 +                # ar should not be change.
  25 +                # See 
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/156799.html
  26 +                if (NOT TARGET_OS_DARWIN)
  27 +                    # llvm-ar is just a wrapper over ar to pass 
--plugin= option,
  28 +                    # so that ar can understand LTO objects.
  29 +                    # See 
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
  30 +                    set (CMAKE_AR llvm-ar)
  31 + endif()
  32 else()
  33 -`......# GNU opts to support lto
  34 -`......`.......# Due to some problems (bugs, slow work, etc) we 
support LTO only for 5.0+
  35 -`......`.......# The same is for binutils prior 2.27
  36 -`......`.......# See https://patchwork.kernel.org/patch/10078207/
  37 +        # GNU opts to support lto
  38 +            # Due to some problems (bugs, slow work, etc) we 
support LTO
  39 +            # only for 5.0+. The same is for binutils prior to 2.27
  40 +            # See comments in scripts/Makefile.lto in scope of
  41 +            # the patch: https://patchwork.kernel.org/patch/10078207/
  42              if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
  43                  AND NOT ${linker_version} VERSION_LESS 2.27)
  44                  set (luajit_cflags ${luajit_cflags} -flto 
-fuse-linker-plugin -fno-fat-lto-objects)

  45 -            endif()

  46 -`......    # The same as for llvm-ar
  47 -`......    # See, https://bugzilla.redhat.com/show_bug.cgi?id=1678826
  48 + endif()
  49 +            # gcc-ar is just a wrapper over ar to pass --plugin 
option
  50 +            # so ar can understand LTO objects. For further info see
  51 +            # -flto and -ffat-lto-objects options descriptions:
  52 +            # 
https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
  53              set (CMAKE_AR gcc-ar)
  54 endif()
  55      endif()

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Igor Munkin]

Olya,

Thanks for the changes!

On 17.04.20, Olga Arkhangelskaia wrote:
> Hi Igor!
> 
> It seems that I eventually had fixed white-space mess.

Yes, there are no tabs in the current patch! However there is a
misindent in the DARWIN-related part (see comments below).

> 
> While testing everything once again I came across reason why I separated 
> case with Darwin and
> 
> llvm-ar. Fixed and added a comment.
> 
> Diffs are below:
> 
> P.S.
> 
> Please, let me know whether new diff is better.

Sorry, but it's not. We can discuss it in tg to not spoil the ml.

<snipped>

Here is the update patch from the upstream:

================================================================================

From: Olga Arkhangelskaia <arkholga@tarantool.org>
Date: Tue, 3 Mar 2020 12:05:49 +0300
Subject: [PATCH] cmake: add LTO support for building luajit

Tarantool has LTO support, however while building luajit this opt. was
omitted. Patch adds necessary flag to turn it on.

Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
errors and slow work.

Closes #3743

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 cmake/luajit.cmake | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 072db8269..d02c35432 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -225,6 +225,45 @@ macro(luajit_build)
         set(luajit_xcflags ${luajit_xcflags} -D${def})
     endforeach()
 
+    # Add LTO option to luajit
+    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
+        message("Setting LTO flags for building luajit")
+        # Clang opt to support LTO
+        if (CMAKE_COMPILER_IS_CLANG)
+            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
+                set (luajit_cflags ${luajit_cflags} -flto=full)
+            else()
+                # ThinLTO that is both scalable and incremental
+                # due to parallel IPO, available since 3.9 and above.
+                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
+                set (luajit_cflags ${luajit_cflags} -flto=thin)
+            endif()
+                # Since Darwin uses DYLD_LIBRARY_PATH instead of llvm-ar
+                # ar should not be change.
+                # See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/156799.html

I propose to mention the original patch[1] instead of the reply. By the
way, this fix differs from the one you mentioned: DYLD_LIBRARY_PATH is
set in the patch, but you just omit changing CMAKE_AR. Whether this fix
is related to our problem? Additionaly this if scope below is
misindented, please adjust it.

+                if (NOT TARGET_OS_DARWIN)
+                    # llvm-ar is just a wrapper over ar to pass --plugin= option,
+                    # so that ar can understand LTO objects.
+                    # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
+                    set (CMAKE_AR llvm-ar)
+                endif()
+        else()
+        # GNU opts to support lto
+            # Due to some problems (bugs, slow work, etc) we support LTO
+            # only for 5.0+. The same is for binutils prior to 2.27
+            # See comments in scripts/Makefile.lto in scope of
+            # the patch: https://patchwork.kernel.org/patch/10078207/
+            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
+                AND NOT ${linker_version} VERSION_LESS 2.27)
+                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
+             endif()
+            # gcc-ar is just a wrapper over ar to pass --plugin option
+            # so ar can understand LTO objects. For further info see
+            # -flto and -ffat-lto-objects options descriptions:
+            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
+            set (CMAKE_AR gcc-ar)
+        endif()
+    endif()
     # Pass the same toolchain that is used for building of
     # tarantool itself, because tools from different toolchains
     # can be incompatible. A compiler and a linker are already set
-- 
2.25.0

================================================================================

> 

[1]: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/156753.html

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Hello Igor!

Thank you for your patience and help.

I fixed previous comments:

   1 diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
   2 index d02c35432..473bfc5e9 100644
   3 --- a/cmake/luajit.cmake
   4 +++ b/cmake/luajit.cmake
   5 @@ -238,15 +238,15 @@ macro(luajit_build)
   6                  # See 
http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
   7                  set (luajit_cflags ${luajit_cflags} -flto=thin)
   8 endif()
   9 -                # Since Darwin uses DYLD_LIBRARY_PATH instead of 
llvm-ar
  10 -                # ar should not be change.
  11 -                # See 
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/156799.html
  12 -                if (NOT TARGET_OS_DARWIN)
  13 -                    # llvm-ar is just a wrapper over ar to pass 
--plugin= option,
  14 -                    # so that ar can understand LTO objects.
  15 -                    # See 
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
  16 -                    set (CMAKE_AR llvm-ar)
  17 - endif()
  18 +                # XCode linker dynamically load libLTO.dylib to 
perform
  19 +                # link-time optimization.
  20 +                # 
http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
  21 +            if (NOT TARGET_OS_DARWIN)
  22 +                # llvm-ar is just a wrapper over ar to pass 
--plugin= option,
  23 +                # so that ar can understand LTO objects.
  24 +                # See 
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
  25 +                set (CMAKE_AR llvm-ar)
  26 + endif()
  27 else()
  28          # GNU opts to support lto
  29              # Due to some problems (bugs, slow work, etc) we 
support LTO

28.04.2020 2:04, Igor Munkin пишет:
> Olya,
>
> Thanks for the changes!
>
> On 17.04.20, Olga Arkhangelskaia wrote:
>> Hi Igor!
>>
>> It seems that I eventually had fixed white-space mess.
> Yes, there are no tabs in the current patch! However there is a
> misindent in the DARWIN-related part (see comments below).
>
>> While testing everything once again I came across reason why I separated
>> case with Darwin and
>>
>> llvm-ar. Fixed and added a comment.
>>
>> Diffs are below:
>>
>> P.S.
>>
>> Please, let me know whether new diff is better.
> Sorry, but it's not. We can discuss it in tg to not spoil the ml.
>
> <snipped>
>
> Here is the update patch from the upstream:
>
> ================================================================================
>
> From: Olga Arkhangelskaia <arkholga@tarantool.org>
> Date: Tue, 3 Mar 2020 12:05:49 +0300
> Subject: [PATCH] cmake: add LTO support for building luajit
>
> Tarantool has LTO support, however while building luajit this opt. was
> omitted. Patch adds necessary flag to turn it on.
>
> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
> errors and slow work.
>
> Closes #3743
>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>   cmake/luajit.cmake | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
>
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 072db8269..d02c35432 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -225,6 +225,45 @@ macro(luajit_build)
>           set(luajit_xcflags ${luajit_xcflags} -D${def})
>       endforeach()
>   
> +    # Add LTO option to luajit
> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> +        message("Setting LTO flags for building luajit")
> +        # Clang opt to support LTO
> +        if (CMAKE_COMPILER_IS_CLANG)
> +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +                set (luajit_cflags ${luajit_cflags} -flto=full)
> +            else()
> +                # ThinLTO that is both scalable and incremental
> +                # due to parallel IPO, available since 3.9 and above.
> +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
> +                set (luajit_cflags ${luajit_cflags} -flto=thin)
> +            endif()
> +                # Since Darwin uses DYLD_LIBRARY_PATH instead of llvm-ar
> +                # ar should not be change.
> +                # See http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/156799.html
>
> I propose to mention the original patch[1] instead of the reply. By the
> way, this fix differs from the one you mentioned: DYLD_LIBRARY_PATH is
> set in the patch, but you just omit changing CMAKE_AR. Whether this fix
> is related to our problem? Additionaly this if scope below is
> misindented, please adjust it.
>
> +                if (NOT TARGET_OS_DARWIN)
> +                    # llvm-ar is just a wrapper over ar to pass --plugin= option,
> +                    # so that ar can understand LTO objects.
> +                    # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> +                    set (CMAKE_AR llvm-ar)
> +                endif()
> +        else()
> +        # GNU opts to support lto
> +            # Due to some problems (bugs, slow work, etc) we support LTO
> +            # only for 5.0+. The same is for binutils prior to 2.27
> +            # See comments in scripts/Makefile.lto in scope of
> +            # the patch: https://patchwork.kernel.org/patch/10078207/
> +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> +                AND NOT ${linker_version} VERSION_LESS 2.27)
> +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> +             endif()
> +            # gcc-ar is just a wrapper over ar to pass --plugin option
> +            # so ar can understand LTO objects. For further info see
> +            # -flto and -ffat-lto-objects options descriptions:
> +            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> +            set (CMAKE_AR gcc-ar)
> +        endif()
> +    endif()
>       # Pass the same toolchain that is used for building of
>       # tarantool itself, because tools from different toolchains
>       # can be incompatible. A compiler and a linker are already set

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Sergey Bronnikov]

Olya,

I have reviewed patch in a branch and below my comments.

formatting issues still exist:

+        if (CMAKE_COMPILER_IS_CLANG)
+            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
+                set (luajit_cflags ${luajit_cflags} -flto=full)
+            else()
+                # ThinLTO that is both scalable and incremental
+                # due to parallel IPO, available since 3.9 and above.
+                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
+                set (luajit_cflags ${luajit_cflags} -flto=thin)
+            endif()
+                # XCode linker dynamically load libLTO.dylib to perform
+                # link-time optimization.
+                # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
^^^^^^^^^^^^^^^^^^^^^^^^

+            if (NOT TARGET_OS_DARWIN)
+                # llvm-ar is just a wrapper over ar to pass --plugin= option,
+                # so that ar can understand LTO objects.
+                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
+                set (CMAKE_AR llvm-ar)
+            endif()
+        else()
+        # GNU opts to support lto
^^^^^^^^^^^^^^^^^^^^^^

+            # Due to some problems (bugs, slow work, etc) we support LTO
+            # only for 5.0+. The same is for binutils prior to 2.27
+            # See comments in scripts/Makefile.lto in scope of


+            # the patch: https://patchwork.kernel.org/patch/10078207/
+            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
+                AND NOT ${linker_version} VERSION_LESS 2.27)
+                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
+             endif()
^^^^^^^^^^^^^^^^^^^^^^
+            # gcc-ar is just a wrapper over ar to pass --plugin option

Unfortunatlely I have no minimal required version of binutils on my system
and I tested patch with ld gold (cmake -DCMAKE_LINKER=/usr/bin/ld.gold ..),
as Timur Safin advised.

How I tested:
$ mkdir build; cd build
$ make build_bundled_libs VERBOSE=1 2>&1 | tee log.txt

with commands above output in log.txt doesn't contain LTO related options
in compilation lines for luajit.

gcc 8.3.1, clang 8.0.1, CentOS 8.1.1911

Sergey

On 17:33 Tue 14 Apr , Olga Arkhangelskaia wrote:
> Fixed tabs:
> 
>  +            # Depending of compiler we need to set appropriate llvm/gcc-ar
> lib
>  +            # llvm/gcc -ar is just a wrapper over ar to pass --plugin=
> option,
>  +            # so that ar can understand LTO objects.
> 
> 14.04.2020 12:18, Igor Munkin пишет:
> > Olya,
> > 
> > Thanks for the patch! I left several comments below, please consider
> > them.
> > 
> > On 12.03.20, Olga Arkhangelskaia wrote:
> > > Tarantool has LTO support, however while building luajit this opt. was
> > > omitted. Patch adds necessary flag to turn it on.
> > > 
> > > Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> > > 
> > > Closes #3743
> > > ---
> > > Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
> > >   cmake/lto.cmake    |  3 ++-
> > >   cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
> > >   2 files changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/cmake/lto.cmake b/cmake/lto.cmake
> > > index 95ade75f4..9f29f3324 100644
> > > --- a/cmake/lto.cmake
> > > +++ b/cmake/lto.cmake
> > > @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
> > >           if (linker_version VERSION_LESS "2.31")
> > >               message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
> > >           endif()
> > > -    elseif(matched_gold)
> > > +
> > > +	elseif(matched_gold)
> > Typo: there is a mess with whitespace above.
> > 
> > However this chunk provides no changes except whitespace and newline.
> > Please drop it.
> > 
> > >           set(linker_version ${CMAKE_MATCH_1})
> > >           message(STATUS "Found ld.gold version: ${linker_version}")
> > > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> > > index 072db8269..da1b4926d 100644
> > > --- a/cmake/luajit.cmake
> > > +++ b/cmake/luajit.cmake
> > > @@ -225,6 +225,32 @@ macro(luajit_build)
> > >           set(luajit_xcflags ${luajit_xcflags} -D${def})
> > >       endforeach()
> > > +    # Add LTO option to luajit
> > > +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> > > +	    message("Setting LTO flags for building luajit")
> > > +	# Clang opt to support LTO
> > > +	if (CMAKE_COMPILER_IS_CLANG AND
> > > +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
> > So, if clang toolchain is used but version is less than 3.4, then gcc
> > toolchain settings are used. It's either totally wrong or definitely
> > need to be clarified with a corresponding comment.
> > 
> > Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
> > CMake module. So additional check here looks to be excess or also need
> > to be clarified.
> > 
> > > +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> > > +		set(luajit_cflags ${luajit_cflags} -flto=full)
> > > +	    else()
> > > +	        set(luajit_cflags ${luajit_cflags} -flto=full)
> > > +	    endif()
> > I see no difference in the branches above.
> > 
> > > +	if (NOT TARGET_OS_DARWIN)
> > > +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
> > > +	   set (CMAKE_AR llvm-ar)
> > What ar is used on systems different from Darwin based ones when clang
> > toolchain is used?
> > 
> > > +	endif()
> > > +	# GNU opts to support lto
> > > +	else()
> > > +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
> > > +	   #The same is for binutils prior 2.27
> > > +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> > > +	       AND NOT ${linker_version} VERSION_LESS 2.27)
> > > +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> > > +	   endif()
> > > +	   set (CMAKE_AR gcc-ar)
> > Could you please clarify why ar is set explicitly here?
> > 
> > > +        endif()
> > > +    endif()
> > Typo: there is a mess with indent above. Unfortunately I failed to find
> > our CMake style guide, so please adjust your changes considering the
> > code nearby.
> > 
> > >       # Pass the same toolchain that is used for building of
> > >       # tarantool itself, because tools from different toolchains
> > >       # can be incompatible. A compiler and a linker are already set
> > > -- 
> > > 2.20.1 (Apple Git-117)
> > > 

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Hello Sergey!

Thanks for review and testing.

However,  -DENABLE_LTO=ON is not set by default, yu have to set it.

I followed your way:

  cmake . -DCMAKE_LINKER=/usr/bin/ld.gold -DENABLE_LTO=ON
-- ld version string: GNU ld version 2.30-58.el8_1.2
-- Found ld.bfd version: 2.30
CMake Error at cmake/lto.cmake:79 (message):
   ld.bfd >= 2.31 is needed for LTO
Call Stack (most recent call first):
   CMakeLists.txt:68 (include)

As far as I understand there gold/bfd is the frontend to binutils, so it 
is impossible om msc.

I did the same at the debian docker container:

Generating third_party/luajit/src/libluajit.a
cd /tarantool/third_party/luajit && make BUILDMODE=static 
HOST_CC="/usr/bin/cc" TARGET_CC="/usr/bin/cc" 
TARGET_CFLAGS="-fexceptions -funwind-tables -fno-omit-frame-pointer 
-fno-stack-protector -fno-common -fopenmp -msse2 
-Wno-parentheses-equality -Wno-tautological-compare 
-Wno-misleading-indentation -Wno-varargs -Wno-implicit-fallthrough -flto 
-fuse-linker-plugin -fno-fat-lto-objects" TARGET_LD="/usr/bin/ld" 
TARGET_LDFLAGS=" -Wno-lto-type-mismatch" TARGET_AR="gcc-ar rcus" 
TARGET_STRIP="/usr/bin/strip" TARGET_SYS="Linux" CCOPT="-O2" CCDEBUG="" 
XCFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -DNDEBUG -DNVALGRIND 
-DNVALGRIND=1 -DLUAJIT_ENABLE_PAIRSMM=1 -DLUAJIT_SMART_STRINGS=1" Q='' 
MACOSX_DEPLOYMENT_TARGET="" clean

So the -flto is passet to luajit.

I will check formatting once again.

25.05.2020 15:58, Sergey Bronnikov пишет:
> Olya,
>
> I have reviewed patch in a branch and below my comments.
>
> formatting issues still exist:
>
> +        if (CMAKE_COMPILER_IS_CLANG)
> +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +                set (luajit_cflags ${luajit_cflags} -flto=full)
> +            else()
> +                # ThinLTO that is both scalable and incremental
> +                # due to parallel IPO, available since 3.9 and above.
> +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
> +                set (luajit_cflags ${luajit_cflags} -flto=thin)
> +            endif()
> +                # XCode linker dynamically load libLTO.dylib to perform
> +                # link-time optimization.
> +                # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> +            if (NOT TARGET_OS_DARWIN)
> +                # llvm-ar is just a wrapper over ar to pass --plugin= option,
> +                # so that ar can understand LTO objects.
> +                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> +                set (CMAKE_AR llvm-ar)
> +            endif()
> +        else()
> +        # GNU opts to support lto
> ^^^^^^^^^^^^^^^^^^^^^^
>
> +            # Due to some problems (bugs, slow work, etc) we support LTO
> +            # only for 5.0+. The same is for binutils prior to 2.27
> +            # See comments in scripts/Makefile.lto in scope of
>
>
> +            # the patch: https://patchwork.kernel.org/patch/10078207/
> +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> +                AND NOT ${linker_version} VERSION_LESS 2.27)
> +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> +             endif()
> ^^^^^^^^^^^^^^^^^^^^^^
> +            # gcc-ar is just a wrapper over ar to pass --plugin option
>
> Unfortunatlely I have no minimal required version of binutils on my system
> and I tested patch with ld gold (cmake -DCMAKE_LINKER=/usr/bin/ld.gold ..),
> as Timur Safin advised.
>
> How I tested:
> $ mkdir build; cd build
> $ make build_bundled_libs VERBOSE=1 2>&1 | tee log.txt
>
> with commands above output in log.txt doesn't contain LTO related options
> in compilation lines for luajit.
>
> gcc 8.3.1, clang 8.0.1, CentOS 8.1.1911
>
> Sergey
>
> On 17:33 Tue 14 Apr , Olga Arkhangelskaia wrote:
>> Fixed tabs:
>>
>>   +            # Depending of compiler we need to set appropriate llvm/gcc-ar
>> lib
>>   +            # llvm/gcc -ar is just a wrapper over ar to pass --plugin=
>> option,
>>   +            # so that ar can understand LTO objects.
>>
>> 14.04.2020 12:18, Igor Munkin пишет:
>>> Olya,
>>>
>>> Thanks for the patch! I left several comments below, please consider
>>> them.
>>>
>>> On 12.03.20, Olga Arkhangelskaia wrote:
>>>> Tarantool has LTO support, however while building luajit this opt. was
>>>> omitted. Patch adds necessary flag to turn it on.
>>>>
>>>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
>>>>
>>>> Closes #3743
>>>> ---
>>>> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>>>>    cmake/lto.cmake    |  3 ++-
>>>>    cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>>>>    2 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>>>> index 95ade75f4..9f29f3324 100644
>>>> --- a/cmake/lto.cmake
>>>> +++ b/cmake/lto.cmake
>>>> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>>>>            if (linker_version VERSION_LESS "2.31")
>>>>                message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>>>>            endif()
>>>> -    elseif(matched_gold)
>>>> +
>>>> +	elseif(matched_gold)
>>> Typo: there is a mess with whitespace above.
>>>
>>> However this chunk provides no changes except whitespace and newline.
>>> Please drop it.
>>>
>>>>            set(linker_version ${CMAKE_MATCH_1})
>>>>            message(STATUS "Found ld.gold version: ${linker_version}")
>>>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>>>> index 072db8269..da1b4926d 100644
>>>> --- a/cmake/luajit.cmake
>>>> +++ b/cmake/luajit.cmake
>>>> @@ -225,6 +225,32 @@ macro(luajit_build)
>>>>            set(luajit_xcflags ${luajit_xcflags} -D${def})
>>>>        endforeach()
>>>> +    # Add LTO option to luajit
>>>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>>>> +	    message("Setting LTO flags for building luajit")
>>>> +	# Clang opt to support LTO
>>>> +	if (CMAKE_COMPILER_IS_CLANG AND
>>>> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
>>> So, if clang toolchain is used but version is less than 3.4, then gcc
>>> toolchain settings are used. It's either totally wrong or definitely
>>> need to be clarified with a corresponding comment.
>>>
>>> Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
>>> CMake module. So additional check here looks to be excess or also need
>>> to be clarified.
>>>
>>>> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>>>> +		set(luajit_cflags ${luajit_cflags} -flto=full)
>>>> +	    else()
>>>> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
>>>> +	    endif()
>>> I see no difference in the branches above.
>>>
>>>> +	if (NOT TARGET_OS_DARWIN)
>>>> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>>>> +	   set (CMAKE_AR llvm-ar)
>>> What ar is used on systems different from Darwin based ones when clang
>>> toolchain is used?
>>>
>>>> +	endif()
>>>> +	# GNU opts to support lto
>>>> +	else()
>>>> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>>>> +	   #The same is for binutils prior 2.27
>>>> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>>>> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
>>>> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
>>>> +	   endif()
>>>> +	   set (CMAKE_AR gcc-ar)
>>> Could you please clarify why ar is set explicitly here?
>>>
>>>> +        endif()
>>>> +    endif()
>>> Typo: there is a mess with indent above. Unfortunately I failed to find
>>> our CMake style guide, so please adjust your changes considering the
>>> code nearby.
>>>
>>>>        # Pass the same toolchain that is used for building of
>>>>        # tarantool itself, because tools from different toolchains
>>>>        # can be incompatible. A compiler and a linker are already set
>>>> -- 
>>>> 2.20.1 (Apple Git-117)
>>>>

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

New diff is below:

              endif()
-                # XCode linker dynamically load libLTO.dylib to perform
-                # link-time optimization.
-                # 
http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
+            # XCode linker dynamically load libLTO.dylib to perform
+            # link-time optimization.
+            # 
http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
              if (NOT TARGET_OS_DARWIN)
                  # llvm-ar is just a wrapper over ar to pass --plugin= 
option,
                  # so that ar can understand LTO objects.
@@ -248,7 +248,7 @@ macro(luajit_build)
                  set (CMAKE_AR llvm-ar)
              endif()
          else()
-        # GNU opts to support lto
+            # GNU opts to support lto
              # Due to some problems (bugs, slow work, etc) we support LTO
              # only for 5.0+. The same is for binutils prior to 2.27
              # See comments in scripts/Makefile.lto in scope of
@@ -256,7 +256,7 @@ macro(luajit_build)
              if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
                  AND NOT ${linker_version} VERSION_LESS 2.27)
                  set (luajit_cflags ${luajit_cflags} -flto 
-fuse-linker-plugin -fno-fat-lto-objects)
-             endif()
+            endif()

25.05.2020 15:58, Sergey Bronnikov пишет:
> Olya,
>
> I have reviewed patch in a branch and below my comments.
>
> formatting issues still exist:
>
> +        if (CMAKE_COMPILER_IS_CLANG)
> +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +                set (luajit_cflags ${luajit_cflags} -flto=full)
> +            else()
> +                # ThinLTO that is both scalable and incremental
> +                # due to parallel IPO, available since 3.9 and above.
> +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
> +                set (luajit_cflags ${luajit_cflags} -flto=thin)
> +            endif()
> +                # XCode linker dynamically load libLTO.dylib to perform
> +                # link-time optimization.
> +                # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> +            if (NOT TARGET_OS_DARWIN)
> +                # llvm-ar is just a wrapper over ar to pass --plugin= option,
> +                # so that ar can understand LTO objects.
> +                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> +                set (CMAKE_AR llvm-ar)
> +            endif()
> +        else()
> +        # GNU opts to support lto
> ^^^^^^^^^^^^^^^^^^^^^^
>
> +            # Due to some problems (bugs, slow work, etc) we support LTO
> +            # only for 5.0+. The same is for binutils prior to 2.27
> +            # See comments in scripts/Makefile.lto in scope of
>
>
> +            # the patch: https://patchwork.kernel.org/patch/10078207/
> +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> +                AND NOT ${linker_version} VERSION_LESS 2.27)
> +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> +             endif()
> ^^^^^^^^^^^^^^^^^^^^^^
> +            # gcc-ar is just a wrapper over ar to pass --plugin option
>
> Unfortunatlely I have no minimal required version of binutils on my system
> and I tested patch with ld gold (cmake -DCMAKE_LINKER=/usr/bin/ld.gold ..),
> as Timur Safin advised.
>
> How I tested:
> $ mkdir build; cd build
> $ make build_bundled_libs VERBOSE=1 2>&1 | tee log.txt
>
> with commands above output in log.txt doesn't contain LTO related options
> in compilation lines for luajit.
>
> gcc 8.3.1, clang 8.0.1, CentOS 8.1.1911
>
> Sergey
>
> On 17:33 Tue 14 Apr , Olga Arkhangelskaia wrote:
>> Fixed tabs:
>>
>>   +            # Depending of compiler we need to set appropriate llvm/gcc-ar
>> lib
>>   +            # llvm/gcc -ar is just a wrapper over ar to pass --plugin=
>> option,
>>   +            # so that ar can understand LTO objects.
>>
>> 14.04.2020 12:18, Igor Munkin пишет:
>>> Olya,
>>>
>>> Thanks for the patch! I left several comments below, please consider
>>> them.
>>>
>>> On 12.03.20, Olga Arkhangelskaia wrote:
>>>> Tarantool has LTO support, however while building luajit this opt. was
>>>> omitted. Patch adds necessary flag to turn it on.
>>>>
>>>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
>>>>
>>>> Closes #3743
>>>> ---
>>>> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>>>>    cmake/lto.cmake    |  3 ++-
>>>>    cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
>>>>    2 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>>>> index 95ade75f4..9f29f3324 100644
>>>> --- a/cmake/lto.cmake
>>>> +++ b/cmake/lto.cmake
>>>> @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
>>>>            if (linker_version VERSION_LESS "2.31")
>>>>                message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
>>>>            endif()
>>>> -    elseif(matched_gold)
>>>> +
>>>> +	elseif(matched_gold)
>>> Typo: there is a mess with whitespace above.
>>>
>>> However this chunk provides no changes except whitespace and newline.
>>> Please drop it.
>>>
>>>>            set(linker_version ${CMAKE_MATCH_1})
>>>>            message(STATUS "Found ld.gold version: ${linker_version}")
>>>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>>>> index 072db8269..da1b4926d 100644
>>>> --- a/cmake/luajit.cmake
>>>> +++ b/cmake/luajit.cmake
>>>> @@ -225,6 +225,32 @@ macro(luajit_build)
>>>>            set(luajit_xcflags ${luajit_xcflags} -D${def})
>>>>        endforeach()
>>>> +    # Add LTO option to luajit
>>>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>>>> +	    message("Setting LTO flags for building luajit")
>>>> +	# Clang opt to support LTO
>>>> +	if (CMAKE_COMPILER_IS_CLANG AND
>>>> +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
>>> So, if clang toolchain is used but version is less than 3.4, then gcc
>>> toolchain settings are used. It's either totally wrong or definitely
>>> need to be clarified with a corresponding comment.
>>>
>>> Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
>>> CMake module. So additional check here looks to be excess or also need
>>> to be clarified.
>>>
>>>> +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>>>> +		set(luajit_cflags ${luajit_cflags} -flto=full)
>>>> +	    else()
>>>> +	        set(luajit_cflags ${luajit_cflags} -flto=full)
>>>> +	    endif()
>>> I see no difference in the branches above.
>>>
>>>> +	if (NOT TARGET_OS_DARWIN)
>>>> +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
>>>> +	   set (CMAKE_AR llvm-ar)
>>> What ar is used on systems different from Darwin based ones when clang
>>> toolchain is used?
>>>
>>>> +	endif()
>>>> +	# GNU opts to support lto
>>>> +	else()
>>>> +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
>>>> +	   #The same is for binutils prior 2.27
>>>> +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>>>> +	       AND NOT ${linker_version} VERSION_LESS 2.27)
>>>> +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
>>>> +	   endif()
>>>> +	   set (CMAKE_AR gcc-ar)
>>> Could you please clarify why ar is set explicitly here?
>>>
>>>> +        endif()
>>>> +    endif()
>>> Typo: there is a mess with indent above. Unfortunately I failed to find
>>> our CMake style guide, so please adjust your changes considering the
>>> code nearby.
>>>
>>>>        # Pass the same toolchain that is used for building of
>>>>        # tarantool itself, because tools from different toolchains
>>>>        # can be incompatible. A compiler and a linker are already set
>>>> -- 
>>>> 2.20.1 (Apple Git-117)
>>>>

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Sergey Bronnikov]

I can confirm, formatting is fixed now.

LGTM

On 18:12 Mon 25 May , Olga Arkhangelskaia wrote:
> New diff is below:
> 
>              endif()
> -                # XCode linker dynamically load libLTO.dylib to perform
> -                # link-time optimization.
> -                #
> http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> +            # XCode linker dynamically load libLTO.dylib to perform
> +            # link-time optimization.
> +            #
> http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
>              if (NOT TARGET_OS_DARWIN)
>                  # llvm-ar is just a wrapper over ar to pass --plugin=
> option,
>                  # so that ar can understand LTO objects.
> @@ -248,7 +248,7 @@ macro(luajit_build)
>                  set (CMAKE_AR llvm-ar)
>              endif()
>          else()
> -        # GNU opts to support lto
> +            # GNU opts to support lto
>              # Due to some problems (bugs, slow work, etc) we support LTO
>              # only for 5.0+. The same is for binutils prior to 2.27
>              # See comments in scripts/Makefile.lto in scope of
> @@ -256,7 +256,7 @@ macro(luajit_build)
>              if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>                  AND NOT ${linker_version} VERSION_LESS 2.27)
>                  set (luajit_cflags ${luajit_cflags} -flto
> -fuse-linker-plugin -fno-fat-lto-objects)
> -             endif()
> +            endif()
> 
> 25.05.2020 15:58, Sergey Bronnikov пишет:
> > Olya,
> > 
> > I have reviewed patch in a branch and below my comments.
> > 
> > formatting issues still exist:
> > 
> > +        if (CMAKE_COMPILER_IS_CLANG)
> > +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> > +                set (luajit_cflags ${luajit_cflags} -flto=full)
> > +            else()
> > +                # ThinLTO that is both scalable and incremental
> > +                # due to parallel IPO, available since 3.9 and above.
> > +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
> > +                set (luajit_cflags ${luajit_cflags} -flto=thin)
> > +            endif()
> > +                # XCode linker dynamically load libLTO.dylib to perform
> > +                # link-time optimization.
> > +                # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > +            if (NOT TARGET_OS_DARWIN)
> > +                # llvm-ar is just a wrapper over ar to pass --plugin= option,
> > +                # so that ar can understand LTO objects.
> > +                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> > +                set (CMAKE_AR llvm-ar)
> > +            endif()
> > +        else()
> > +        # GNU opts to support lto
> > ^^^^^^^^^^^^^^^^^^^^^^
> > 
> > +            # Due to some problems (bugs, slow work, etc) we support LTO
> > +            # only for 5.0+. The same is for binutils prior to 2.27
> > +            # See comments in scripts/Makefile.lto in scope of
> > 
> > 
> > +            # the patch: https://patchwork.kernel.org/patch/10078207/
> > +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> > +                AND NOT ${linker_version} VERSION_LESS 2.27)
> > +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> > +             endif()
> > ^^^^^^^^^^^^^^^^^^^^^^
> > +            # gcc-ar is just a wrapper over ar to pass --plugin option
> > 
> > Unfortunatlely I have no minimal required version of binutils on my system
> > and I tested patch with ld gold (cmake -DCMAKE_LINKER=/usr/bin/ld.gold ..),
> > as Timur Safin advised.
> > 
> > How I tested:
> > $ mkdir build; cd build
> > $ make build_bundled_libs VERBOSE=1 2>&1 | tee log.txt
> > 
> > with commands above output in log.txt doesn't contain LTO related options
> > in compilation lines for luajit.
> > 
> > gcc 8.3.1, clang 8.0.1, CentOS 8.1.1911
> > 
> > Sergey
> > 
> > On 17:33 Tue 14 Apr , Olga Arkhangelskaia wrote:
> > > Fixed tabs:
> > > 
> > >   +            # Depending of compiler we need to set appropriate llvm/gcc-ar
> > > lib
> > >   +            # llvm/gcc -ar is just a wrapper over ar to pass --plugin=
> > > option,
> > >   +            # so that ar can understand LTO objects.
> > > 
> > > 14.04.2020 12:18, Igor Munkin пишет:
> > > > Olya,
> > > > 
> > > > Thanks for the patch! I left several comments below, please consider
> > > > them.
> > > > 
> > > > On 12.03.20, Olga Arkhangelskaia wrote:
> > > > > Tarantool has LTO support, however while building luajit this opt. was
> > > > > omitted. Patch adds necessary flag to turn it on.
> > > > > 
> > > > > Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> > > > > 
> > > > > Closes #3743
> > > > > ---
> > > > > Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
> > > > >    cmake/lto.cmake    |  3 ++-
> > > > >    cmake/luajit.cmake | 26 ++++++++++++++++++++++++++
> > > > >    2 files changed, 28 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/cmake/lto.cmake b/cmake/lto.cmake
> > > > > index 95ade75f4..9f29f3324 100644
> > > > > --- a/cmake/lto.cmake
> > > > > +++ b/cmake/lto.cmake
> > > > > @@ -78,7 +78,8 @@ if (NOT TARGET_OS_DARWIN)
> > > > >            if (linker_version VERSION_LESS "2.31")
> > > > >                message(FATAL_ERROR "ld.bfd >= 2.31 is needed for LTO")
> > > > >            endif()
> > > > > -    elseif(matched_gold)
> > > > > +
> > > > > +	elseif(matched_gold)
> > > > Typo: there is a mess with whitespace above.
> > > > 
> > > > However this chunk provides no changes except whitespace and newline.
> > > > Please drop it.
> > > > 
> > > > >            set(linker_version ${CMAKE_MATCH_1})
> > > > >            message(STATUS "Found ld.gold version: ${linker_version}")
> > > > > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> > > > > index 072db8269..da1b4926d 100644
> > > > > --- a/cmake/luajit.cmake
> > > > > +++ b/cmake/luajit.cmake
> > > > > @@ -225,6 +225,32 @@ macro(luajit_build)
> > > > >            set(luajit_xcflags ${luajit_xcflags} -D${def})
> > > > >        endforeach()
> > > > > +    # Add LTO option to luajit
> > > > > +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> > > > > +	    message("Setting LTO flags for building luajit")
> > > > > +	# Clang opt to support LTO
> > > > > +	if (CMAKE_COMPILER_IS_CLANG AND
> > > > > +	    NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.4)
> > > > So, if clang toolchain is used but version is less than 3.4, then gcc
> > > > toolchain settings are used. It's either totally wrong or definitely
> > > > need to be clarified with a corresponding comment.
> > > > 
> > > > Also, AFAICS LTO support for toolchain is checked via CheckIPOSupported
> > > > CMake module. So additional check here looks to be excess or also need
> > > > to be clarified.
> > > > 
> > > > > +	    if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> > > > > +		set(luajit_cflags ${luajit_cflags} -flto=full)
> > > > > +	    else()
> > > > > +	        set(luajit_cflags ${luajit_cflags} -flto=full)
> > > > > +	    endif()
> > > > I see no difference in the branches above.
> > > > 
> > > > > +	if (NOT TARGET_OS_DARWIN)
> > > > > +	   # Depending of compiler we need to set appropriate llvm/gcc-ar lib
> > > > > +	   set (CMAKE_AR llvm-ar)
> > > > What ar is used on systems different from Darwin based ones when clang
> > > > toolchain is used?
> > > > 
> > > > > +	endif()
> > > > > +	# GNU opts to support lto
> > > > > +	else()
> > > > > +	   #Due to some problems (bugs, slow work, etc) we support LTO only for 5.0+
> > > > > +	   #The same is for binutils prior 2.27
> > > > > +	   if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> > > > > +	       AND NOT ${linker_version} VERSION_LESS 2.27)
> > > > > +	       set(luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> > > > > +	   endif()
> > > > > +	   set (CMAKE_AR gcc-ar)
> > > > Could you please clarify why ar is set explicitly here?
> > > > 
> > > > > +        endif()
> > > > > +    endif()
> > > > Typo: there is a mess with indent above. Unfortunately I failed to find
> > > > our CMake style guide, so please adjust your changes considering the
> > > > code nearby.
> > > > 
> > > > >        # Pass the same toolchain that is used for building of
> > > > >        # tarantool itself, because tools from different toolchains
> > > > >        # can be incompatible. A compiler and a linker are already set
> > > > > -- 
> > > > > 2.20.1 (Apple Git-117)
> > > > > 

-- 
sergeyb@

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Igor Munkin]

Olya,

Thanks, the patch is nice! I also checked it manually on my Gentoo host
the way Sergey and you proposed:
| $ cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_LTO=ON . &> cmake.out
| $ grep -F 'LTO' cmake.out
| -- Enabling LTO: TRUE
| Setting LTO flags for building luajit
| $ make VERBOSE=1 &> make.out
| $ grep -F 'lj_vm.o' make.out
| ASM       lj_vm.o
| /usr/bin/cc -O0 -Wall -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -DNVALGRIND=1
| -DLUA_USE_APICHECK=1 -DLUA_USE_ASSERT=1 -DLUAJIT_ENABLE_PAIRSMM=1
| -DLUAJIT_SMART_STRINGS=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
| -U_FORTIFY_SOURCE -fno-stack-protector  -fexceptions -funwind-tables
| -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2
| -Wno-parentheses-equality -Wno-tautological-compare
| -Wno-misleading-indentation -Wno-varargs -Wno-implicit-fallthrough -flto
| -fuse-linker-plugin -fno-fat-lto-objects -c -o lj_vm.o lj_vm.S
| gcc-ar rcus libluajit.a lj_vm.o lj_gc.o lj_err.o lj_char.o lj_bc.o
| lj_obj.o lj_buf.o lj_str.o lj_tab.o lj_func.o lj_udata.o lj_meta.o
| lj_debug.o lj_state.o lj_dispatch.o lj_vmevent.o lj_vmmath.o lj_strscan.o
| lj_strfmt.o lj_strfmt_num.o lj_api.o lj_profile.o lj_lex.o lj_parse.o
| lj_bcread.o lj_bcwrite.o lj_load.o lj_ir.o lj_opt_mem.o lj_opt_fold.o
| lj_opt_narrow.o lj_opt_dce.o lj_opt_loop.o lj_opt_split.o lj_opt_sink.o
| lj_mcode.o lj_snap.o lj_record.o lj_crecord.o lj_ffrecord.o lj_asm.o
| lj_trace.o lj_gdbjit.o lj_ctype.o lj_cdata.o lj_cconv.o lj_ccall.o
| lj_ccallback.o lj_carith.o lj_clib.o lj_cparse.o lj_lib.o lj_alloc.o
| lib_aux.o lib_base.o lib_math.o lib_bit.o lib_string.o lib_table.o
| lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o lib_init.o

I see all requested build options are set. However, I see build failures
on OSX[1] not directly related to the changes. So I propose to rebase
your patch on the current master and re-run full CI pipeline to make
sure there are no build failures related to your patch (but I don't
expect any).

Please also provide a ChangeLog entry.

Otherwise, the changes LGTM, thanks!

Sasha, please proceed with the patch.

<snipped>

[1]: https://gitlab.com/tarantool/tarantool/-/jobs/567445297

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

[-- Attachment #1: Type: text/plain, Size: 2580 bytes --]

Hi Igor!

I have rebased to master:

https://gitlab.com/tarantool/tarantool/pipelines/150002628

Unfortunately, release_asan_clang8  fails, however it is not only my 
problem.

Moreover, fails not buid but a test.

@ChangeLog

-flag -DENABLE_LTO=ON is passed to luajit



26.05.2020 13:11, Igor Munkin пишет:
> Olya,
>
> Thanks, the patch is nice! I also checked it manually on my Gentoo host
> the way Sergey and you proposed:
> | $ cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_LTO=ON . &> cmake.out
> | $ grep -F 'LTO' cmake.out
> | -- Enabling LTO: TRUE
> | Setting LTO flags for building luajit
> | $ make VERBOSE=1 &> make.out
> | $ grep -F 'lj_vm.o' make.out
> | ASM       lj_vm.o
> | /usr/bin/cc -O0 -Wall -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -DNVALGRIND=1
> | -DLUA_USE_APICHECK=1 -DLUA_USE_ASSERT=1 -DLUAJIT_ENABLE_PAIRSMM=1
> | -DLUAJIT_SMART_STRINGS=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> | -U_FORTIFY_SOURCE -fno-stack-protector  -fexceptions -funwind-tables
> | -fno-omit-frame-pointer -fno-stack-protector -fno-common -fopenmp -msse2
> | -Wno-parentheses-equality -Wno-tautological-compare
> | -Wno-misleading-indentation -Wno-varargs -Wno-implicit-fallthrough -flto
> | -fuse-linker-plugin -fno-fat-lto-objects -c -o lj_vm.o lj_vm.S
> | gcc-ar rcus libluajit.a lj_vm.o lj_gc.o lj_err.o lj_char.o lj_bc.o
> | lj_obj.o lj_buf.o lj_str.o lj_tab.o lj_func.o lj_udata.o lj_meta.o
> | lj_debug.o lj_state.o lj_dispatch.o lj_vmevent.o lj_vmmath.o lj_strscan.o
> | lj_strfmt.o lj_strfmt_num.o lj_api.o lj_profile.o lj_lex.o lj_parse.o
> | lj_bcread.o lj_bcwrite.o lj_load.o lj_ir.o lj_opt_mem.o lj_opt_fold.o
> | lj_opt_narrow.o lj_opt_dce.o lj_opt_loop.o lj_opt_split.o lj_opt_sink.o
> | lj_mcode.o lj_snap.o lj_record.o lj_crecord.o lj_ffrecord.o lj_asm.o
> | lj_trace.o lj_gdbjit.o lj_ctype.o lj_cdata.o lj_cconv.o lj_ccall.o
> | lj_ccallback.o lj_carith.o lj_clib.o lj_cparse.o lj_lib.o lj_alloc.o
> | lib_aux.o lib_base.o lib_math.o lib_bit.o lib_string.o lib_table.o
> | lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o lib_init.o
>
> I see all requested build options are set. However, I see build failures
> on OSX[1] not directly related to the changes. So I propose to rebase
> your patch on the current master and re-run full CI pipeline to make
> sure there are no build failures related to your patch (but I don't
> expect any).
>
> Please also provide a ChangeLog entry.
>
> Otherwise, the changes LGTM, thanks!
>
> Sasha, please proceed with the patch.
>
> <snipped>
>
> [1]: https://gitlab.com/tarantool/tarantool/-/jobs/567445297
>

[-- Attachment #2: Type: text/html, Size: 4095 bytes --]

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Alexander Turenko]

I vote to use the same build tools and flags that CMake uses to build
files under its control, because otherwise we would write the same logic
again. It may be not as accurate as one that CMake provides (see example
below). It may become outdated in a future. And, last but not least,
duplicated code is painful to maintain.

However the way I proposed requires to use undocumented CMake variables
(see [0]). But I think that's evil of the lesser kind.

I think it would be good to expose related build tool names and flags
from cmake/lto.cmake and use them in cmake/luajit.cmake. I implemented
the former part (I would even left STATUS messages as is, they provide
useful information):

 | diff --git a/cmake/lto.cmake b/cmake/lto.cmake
 | index 95ade75f4..79b908e26 100644
 | --- a/cmake/lto.cmake
 | +++ b/cmake/lto.cmake
 | @@ -90,8 +90,40 @@ if (NOT TARGET_OS_DARWIN)
 |      endif()
 |  endif()
 |  
 | -# gh-3742: investigate LTO warnings.
 | -set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
 | +# {{{ Expose build tools and flags
 | +#
 | +# It is convenient for building non-cmake targets with the same
 | +# flags as we use for sources under CMake control.
 | +#
 | +# It leans on uncodumented variables that are set in the following
 | +# CMake modules: Compiler/GNU.cmake and Compiler/Clang.cmake.
 | +
 | +# CFLAGS_LTO (list)
 | +set(CFLAGS_LTO ${CMAKE_C_COMPILE_OPTIONS_IPO})
 | +message(STATUS "CFLAGS_LTO: ${CFLAGS_LTO}")
 |  
 | +# LDFLAGS_LTO (list)
 | +set(LDFLAGS_LTO ${CMAKE_C_LINK_OPTIONS_IPO})
 | +# FIXME: gh-3742: investigate LTO warnings.
 | +list(APPEND LDFLAGS_LTO -Wno-lto-type-mismatch)
 | +message(STATUS "LDFLAGS_LTO: ${LDFLAGS_LTO}")
 | +
 | +# AR_LTO (string)
 | +#
 | +# Note: Platform/Linux-Intel.cmake and Platform/Windows-MSVC.cmake
 | +# set CMAKE_C_CREATE_STATIC_LIBRARY_IPO, but not
 | +# CMAKE_C_ARCHIVE_CREATE_IPO. So this snippet is only for GCC and
 | +# clang.
 | +set(_ar_command ${CMAKE_C_ARCHIVE_CREATE_IPO})
 | +separate_arguments(_ar_command)
 | +list(GET _ar_command 0 AR_LTO)
 | +unset(_ar_command)
 | +message(STATUS "AR_LTO: ${AR_LTO}")
 | +
 | +# }}}
 | +
 | +# Set build tools and flags for files that are built using CMake.
 | +set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
 |  set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
 | +
 |  message(STATUS "Enabling LTO: TRUE")

Olga, Igor, what do you think about this way?

Or, maybe, we should just add LJCORE_O=ljamalg.o to the Make command
(just like `make amalg` does)? I was curious and tried it:

 | diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
 | index dd9d748ca..d3df66652 100644
 | --- a/cmake/luajit.cmake
 | +++ b/cmake/luajit.cmake
 | @@ -284,6 +284,7 @@ macro(luajit_build)
 |          CCOPT="${luajit_ccopt}"
 |          CCDEBUG="${luajit_ccdebug}"
 |          XCFLAGS="${luajit_xcflags}"
 | +        LJCORE_O=ljamalg.o
 |          Q=''
 |          # We need to set MACOSX_DEPLOYMENT_TARGET to at least 10.6,
 |          # because 10.4 SDK (which is set by default in LuaJIT's

And got undefined reference errors from src/lua/utils.c code that use
LuaJIT internals.

Despite way we'll choose, I would try some benchmarks with jit enabled
and disabled (`jit.off()`) to see whether compiling of LuaJIT with LTO
gives some gains. Technically it is not part of the task (my bad), so
skip it if you're not curious. There is the good candidate to look into:
[3].

Placed the actual version from the branch [1] to comment it.

> commit 62e5d0af7f7a5e44f27b741d43c6c035f20e93b4
> Author: Olga Arkhangelskaia <arkholga@tarantool.org>
> Date:   Tue Mar 3 12:05:49 2020 +0300
> 
>     cmake: add LTO support for building luajit
>     
>     Tarantool has LTO support, however while building luajit this opt. was
>     omitted. Patch adds necessary flag to turn it on.
>     
>     Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
>     errors and slow work.
>     
>     Closes #3743
> 
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 10df633d5..dd9d748ca 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -224,6 +224,45 @@ macro(luajit_build)
>          set(luajit_xcflags ${luajit_xcflags} -D${def})
>      endforeach()
>  
> +    # Add LTO option to luajit
> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)

I would lean on ENABLE_LTO variable and the fact that all necessary
checks already performed by cmake/lto.cmake. I guess it would be more
intuitive.

> +        message("Setting LTO flags for building luajit")

I would use STATUS mode, the message is informational and should not
attract user's attention. STATUS mode is used for such messages in
cmake/lto.cmake. See [2] for information about message types.

> +        # Clang opt to support LTO
> +        if (CMAKE_COMPILER_IS_CLANG)
> +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> +                set (luajit_cflags ${luajit_cflags} -flto=full)
> +            else()
> +                # ThinLTO that is both scalable and incremental
> +                # due to parallel IPO, available since 3.9 and above.
> +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
> +                set (luajit_cflags ${luajit_cflags} -flto=thin)
> +            endif()
> +            # XCode linker dynamically load libLTO.dylib to perform
> +            # link-time optimization.
> +            # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> +            if (NOT TARGET_OS_DARWIN)
> +                # llvm-ar is just a wrapper over ar to pass --plugin= option,
> +                # so that ar can understand LTO objects.
> +                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> +                set (CMAKE_AR llvm-ar)
> +            endif()

I guess it reflects the logic of the following snippet from
Compiler/Clang.cmake CMake module:

 | string(COMPARE EQUAL "${CMAKE_${lang}_COMPILER_ID}" "AppleClang" __is_apple_clang)
 | <...>
 | if(ANDROID OR __is_apple_clang)
 |   set(__ar "${CMAKE_AR}")
 |   set(__ranlib "${CMAKE_RANLIB}")
 | else()
 |   set(__ar "${CMAKE_${lang}_COMPILER_AR}")
 |   set(__ranlib "${CMAKE_${lang}_COMPILER_RANLIB}")
 | endif()
 |
 | set(CMAKE_${lang}_ARCHIVE_CREATE_IPO
 |   "\"${__ar}\" cr <TARGET> <LINK_FLAGS> <OBJECTS>"
 | )
 |
 | set(CMAKE_${lang}_ARCHIVE_APPEND_IPO
 |   "\"${__ar}\" r <TARGET> <LINK_FLAGS> <OBJECTS>"
 | )
 |
 | set(CMAKE_${lang}_ARCHIVE_FINISH_IPO
 |   "\"${__ranlib}\" <TARGET>"
 | )

So TARGET_OS_DARWIN check is not accurate: we should check whether Clang
is built by Apple or, say, installed from brew; not just OS.

> +        else()
> +            # GNU opts to support lto
> +            # Due to some problems (bugs, slow work, etc) we support LTO
> +            # only for 5.0+. The same is for binutils prior to 2.27
> +            # See comments in scripts/Makefile.lto in scope of
> +            # the patch: https://patchwork.kernel.org/patch/10078207/
> +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> +                AND NOT ${linker_version} VERSION_LESS 2.27)
> +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> +            endif()
> +            # gcc-ar is just a wrapper over ar to pass --plugin option
> +            # so ar can understand LTO objects. For further info see
> +            # -flto and -ffat-lto-objects options descriptions:
> +            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> +            set (CMAKE_AR gcc-ar)
> +        endif()
> +    endif()
>      # Pass the same toolchain that is used for building of
>      # tarantool itself, because tools from different toolchains
>      # can be incompatible. A compiler and a linker are already set

[0]: https://gitlab.kitware.com/cmake/cmake/-/issues/16808
[1]: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
[2]: https://cmake.org/cmake/help/latest/command/message.html
[3]: https://gitspartv.github.io/LuaJIT-Benchmarks/

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Hi!

Alexander! I am very thankful for the detailed review of the issue.

I have spent a lot of time reading about LTO and how it is supported in 
others projects. Ad did not

think about cmake itself.

I have applied your patch and edit luajit.cmake according to newly 
exposed variables. It has changed completely.

Diff:

-    # Add LTO option to luajit
-    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
-        message("Setting LTO flags for building luajit")
-        # Clang opt to support LTO
-        if (CMAKE_COMPILER_IS_CLANG)
-            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
-                set (luajit_cflags ${luajit_cflags} -flto=full)
- else()
-                # ThinLTO that is both scalable and incremental
-                # due to parallel IPO, available since 3.9 and above.
-                # See 
http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
-                set (luajit_cflags ${luajit_cflags} -flto=thin)
- endif()
-            # XCode linker dynamically load libLTO.dylib to perform
-            # link-time optimization.
-            # 
http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
-            if (NOT TARGET_OS_DARWIN)
-                # llvm-ar is just a wrapper over ar to pass --plugin= 
option,
-                # so that ar can understand LTO objects.
-                # See 
https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
-                set (CMAKE_AR llvm-ar)
- endif()
- else()
-            # GNU opts to support lto
-            # Due to some problems (bugs, slow work, etc) we support LTO
-            # only for 5.0+. The same is for binutils prior to 2.27
-            # See comments in scripts/Makefile.lto in scope of
-            # the patch: https://patchwork.kernel.org/patch/10078207/
-            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
-                AND NOT ${linker_version} VERSION_LESS 2.27)
-                set (luajit_cflags ${luajit_cflags} -flto 
-fuse-linker-plugin -fno-fat-lto-objects)
- endif()
-            # gcc-ar is just a wrapper over ar to pass --plugin option
-            # so ar can understand LTO objects. For further info see
-            # -flto and -ffat-lto-objects options descriptions:
-            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
-            set (CMAKE_AR gcc-ar)
- endif()
+    # Enablibg LTO for luajit if DENABLE_LTO set.
+    if (${ENABLE_LTO})
+`......message(STATUS "Enablig LTO for luajit")
+        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})
+`......set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
+`......set (CMAKE_AR ${AR_LTO})
endif()
-    # Pass the same toolchain that is used for building of

I have test it with MacOS  and debian. And will do full-ci for it.

16.06.2020 4:02, Alexander Turenko пишет:
> I vote to use the same build tools and flags that CMake uses to build
> files under its control, because otherwise we would write the same logic
> again. It may be not as accurate as one that CMake provides (see example
> below). It may become outdated in a future. And, last but not least,
> duplicated code is painful to maintain.
>
> However the way I proposed requires to use undocumented CMake variables
> (see [0]). But I think that's evil of the lesser kind.
>
> I think it would be good to expose related build tool names and flags
> from cmake/lto.cmake and use them in cmake/luajit.cmake. I implemented
> the former part (I would even left STATUS messages as is, they provide
> useful information):
>
>   | diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>   | index 95ade75f4..79b908e26 100644
>   | --- a/cmake/lto.cmake
>   | +++ b/cmake/lto.cmake
>   | @@ -90,8 +90,40 @@ if (NOT TARGET_OS_DARWIN)
>   |      endif()
>   |  endif()
>   |
>   | -# gh-3742: investigate LTO warnings.
>   | -set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
>   | +# {{{ Expose build tools and flags
>   | +#
>   | +# It is convenient for building non-cmake targets with the same
>   | +# flags as we use for sources under CMake control.
>   | +#
>   | +# It leans on uncodumented variables that are set in the following
>   | +# CMake modules: Compiler/GNU.cmake and Compiler/Clang.cmake.
>   | +
>   | +# CFLAGS_LTO (list)
>   | +set(CFLAGS_LTO ${CMAKE_C_COMPILE_OPTIONS_IPO})
>   | +message(STATUS "CFLAGS_LTO: ${CFLAGS_LTO}")
>   |
>   | +# LDFLAGS_LTO (list)
>   | +set(LDFLAGS_LTO ${CMAKE_C_LINK_OPTIONS_IPO})
>   | +# FIXME: gh-3742: investigate LTO warnings.
>   | +list(APPEND LDFLAGS_LTO -Wno-lto-type-mismatch)
>   | +message(STATUS "LDFLAGS_LTO: ${LDFLAGS_LTO}")
>   | +
>   | +# AR_LTO (string)
>   | +#
>   | +# Note: Platform/Linux-Intel.cmake and Platform/Windows-MSVC.cmake
>   | +# set CMAKE_C_CREATE_STATIC_LIBRARY_IPO, but not
>   | +# CMAKE_C_ARCHIVE_CREATE_IPO. So this snippet is only for GCC and
>   | +# clang.
>   | +set(_ar_command ${CMAKE_C_ARCHIVE_CREATE_IPO})
>   | +separate_arguments(_ar_command)
>   | +list(GET _ar_command 0 AR_LTO)
>   | +unset(_ar_command)
>   | +message(STATUS "AR_LTO: ${AR_LTO}")
>   | +
>   | +# }}}
>   | +
>   | +# Set build tools and flags for files that are built using CMake.
>   | +set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
>   |  set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
>   | +
>   |  message(STATUS "Enabling LTO: TRUE")
>
> Olga, Igor, what do you think about this way?
>
> Or, maybe, we should just add LJCORE_O=ljamalg.o to the Make command
> (just like `make amalg` does)? I was curious and tried it:
>
>   | diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>   | index dd9d748ca..d3df66652 100644
>   | --- a/cmake/luajit.cmake
>   | +++ b/cmake/luajit.cmake
>   | @@ -284,6 +284,7 @@ macro(luajit_build)
>   |          CCOPT="${luajit_ccopt}"
>   |          CCDEBUG="${luajit_ccdebug}"
>   |          XCFLAGS="${luajit_xcflags}"
>   | +        LJCORE_O=ljamalg.o
>   |          Q=''
>   |          # We need to set MACOSX_DEPLOYMENT_TARGET to at least 10.6,
>   |          # because 10.4 SDK (which is set by default in LuaJIT's
>
> And got undefined reference errors from src/lua/utils.c code that use
> LuaJIT internals.
>
> Despite way we'll choose, I would try some benchmarks with jit enabled
> and disabled (`jit.off()`) to see whether compiling of LuaJIT with LTO
> gives some gains. Technically it is not part of the task (my bad), so
> skip it if you're not curious. There is the good candidate to look into:
> [3].
>
> Placed the actual version from the branch [1] to comment it.
>
>> commit 62e5d0af7f7a5e44f27b741d43c6c035f20e93b4
>> Author: Olga Arkhangelskaia <arkholga@tarantool.org>
>> Date:   Tue Mar 3 12:05:49 2020 +0300
>>
>>      cmake: add LTO support for building luajit
>>      
>>      Tarantool has LTO support, however while building luajit this opt. was
>>      omitted. Patch adds necessary flag to turn it on.
>>      
>>      Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
>>      errors and slow work.
>>      
>>      Closes #3743
>>
>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>> index 10df633d5..dd9d748ca 100644
>> --- a/cmake/luajit.cmake
>> +++ b/cmake/luajit.cmake
>> @@ -224,6 +224,45 @@ macro(luajit_build)
>>           set(luajit_xcflags ${luajit_xcflags} -D${def})
>>       endforeach()
>>   
>> +    # Add LTO option to luajit
>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> I would lean on ENABLE_LTO variable and the fact that all necessary
> checks already performed by cmake/lto.cmake. I guess it would be more
> intuitive.
>
>> +        message("Setting LTO flags for building luajit")
> I would use STATUS mode, the message is informational and should not
> attract user's attention. STATUS mode is used for such messages in
> cmake/lto.cmake. See [2] for information about message types.
>
>> +        # Clang opt to support LTO
>> +        if (CMAKE_COMPILER_IS_CLANG)
>> +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>> +                set (luajit_cflags ${luajit_cflags} -flto=full)
>> +            else()
>> +                # ThinLTO that is both scalable and incremental
>> +                # due to parallel IPO, available since 3.9 and above.
>> +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
>> +                set (luajit_cflags ${luajit_cflags} -flto=thin)
>> +            endif()
>> +            # XCode linker dynamically load libLTO.dylib to perform
>> +            # link-time optimization.
>> +            # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
>> +            if (NOT TARGET_OS_DARWIN)
>> +                # llvm-ar is just a wrapper over ar to pass --plugin= option,
>> +                # so that ar can understand LTO objects.
>> +                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
>> +                set (CMAKE_AR llvm-ar)
>> +            endif()
> I guess it reflects the logic of the following snippet from
> Compiler/Clang.cmake CMake module:
>
>   | string(COMPARE EQUAL "${CMAKE_${lang}_COMPILER_ID}" "AppleClang" __is_apple_clang)
>   | <...>
>   | if(ANDROID OR __is_apple_clang)
>   |   set(__ar "${CMAKE_AR}")
>   |   set(__ranlib "${CMAKE_RANLIB}")
>   | else()
>   |   set(__ar "${CMAKE_${lang}_COMPILER_AR}")
>   |   set(__ranlib "${CMAKE_${lang}_COMPILER_RANLIB}")
>   | endif()
>   |
>   | set(CMAKE_${lang}_ARCHIVE_CREATE_IPO
>   |   "\"${__ar}\" cr <TARGET> <LINK_FLAGS> <OBJECTS>"
>   | )
>   |
>   | set(CMAKE_${lang}_ARCHIVE_APPEND_IPO
>   |   "\"${__ar}\" r <TARGET> <LINK_FLAGS> <OBJECTS>"
>   | )
>   |
>   | set(CMAKE_${lang}_ARCHIVE_FINISH_IPO
>   |   "\"${__ranlib}\" <TARGET>"
>   | )
>
> So TARGET_OS_DARWIN check is not accurate: we should check whether Clang
> is built by Apple or, say, installed from brew; not just OS.
>
>> +        else()
>> +            # GNU opts to support lto
>> +            # Due to some problems (bugs, slow work, etc) we support LTO
>> +            # only for 5.0+. The same is for binutils prior to 2.27
>> +            # See comments in scripts/Makefile.lto in scope of
>> +            # the patch: https://patchwork.kernel.org/patch/10078207/
>> +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>> +                AND NOT ${linker_version} VERSION_LESS 2.27)
>> +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
>> +            endif()
>> +            # gcc-ar is just a wrapper over ar to pass --plugin option
>> +            # so ar can understand LTO objects. For further info see
>> +            # -flto and -ffat-lto-objects options descriptions:
>> +            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
>> +            set (CMAKE_AR gcc-ar)
>> +        endif()
>> +    endif()
>>       # Pass the same toolchain that is used for building of
>>       # tarantool itself, because tools from different toolchains
>>       # can be incompatible. A compiler and a linker are already set
> [0]: https://gitlab.kitware.com/cmake/cmake/-/issues/16808
> [1]: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
> [2]: https://cmake.org/cmake/help/latest/command/message.html
> [3]: https://gitspartv.github.io/LuaJIT-Benchmarks/
>
> WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Update branch and checked output on release_lto and release_lto_clang8.

They are ok.

16.06.2020 14:36, Olga Arkhangelskaia пишет:
> Hi!
>
> Alexander! I am very thankful for the detailed review of the issue.
>
> I have spent a lot of time reading about LTO and how it is supported 
> in others projects. Ad did not
>
> think about cmake itself.
>
> I have applied your patch and edit luajit.cmake according to newly 
> exposed variables. It has changed completely.
>
> Diff:
>
> -    # Add LTO option to luajit
> -    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> -        message("Setting LTO flags for building luajit")
> -        # Clang opt to support LTO
> -        if (CMAKE_COMPILER_IS_CLANG)
> -            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> -                set (luajit_cflags ${luajit_cflags} -flto=full)
> - else()
> -                # ThinLTO that is both scalable and incremental
> -                # due to parallel IPO, available since 3.9 and above.
> -                # See 
> http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
> -                set (luajit_cflags ${luajit_cflags} -flto=thin)
> - endif()
> -            # XCode linker dynamically load libLTO.dylib to perform
> -            # link-time optimization.
> -            # 
> http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> -            if (NOT TARGET_OS_DARWIN)
> -                # llvm-ar is just a wrapper over ar to pass --plugin= 
> option,
> -                # so that ar can understand LTO objects.
> -                # See 
> https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> -                set (CMAKE_AR llvm-ar)
> - endif()
> - else()
> -            # GNU opts to support lto
> -            # Due to some problems (bugs, slow work, etc) we support LTO
> -            # only for 5.0+. The same is for binutils prior to 2.27
> -            # See comments in scripts/Makefile.lto in scope of
> -            # the patch: https://patchwork.kernel.org/patch/10078207/
> -            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> -                AND NOT ${linker_version} VERSION_LESS 2.27)
> -                set (luajit_cflags ${luajit_cflags} -flto 
> -fuse-linker-plugin -fno-fat-lto-objects)
> - endif()
> -            # gcc-ar is just a wrapper over ar to pass --plugin option
> -            # so ar can understand LTO objects. For further info see
> -            # -flto and -ffat-lto-objects options descriptions:
> -            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> -            set (CMAKE_AR gcc-ar)
> - endif()
> +    # Enablibg LTO for luajit if DENABLE_LTO set.
> +    if (${ENABLE_LTO})
> +`......message(STATUS "Enablig LTO for luajit")
> +        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})
> +`......set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
> +`......set (CMAKE_AR ${AR_LTO})
> endif()
> -    # Pass the same toolchain that is used for building of
>
> I have test it with MacOS  and debian. And will do full-ci for it.
>
> 16.06.2020 4:02, Alexander Turenko пишет:
>> I vote to use the same build tools and flags that CMake uses to build
>> files under its control, because otherwise we would write the same logic
>> again. It may be not as accurate as one that CMake provides (see example
>> below). It may become outdated in a future. And, last but not least,
>> duplicated code is painful to maintain.
>>
>> However the way I proposed requires to use undocumented CMake variables
>> (see [0]). But I think that's evil of the lesser kind.
>>
>> I think it would be good to expose related build tool names and flags
>> from cmake/lto.cmake and use them in cmake/luajit.cmake. I implemented
>> the former part (I would even left STATUS messages as is, they provide
>> useful information):
>>
>>   | diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>>   | index 95ade75f4..79b908e26 100644
>>   | --- a/cmake/lto.cmake
>>   | +++ b/cmake/lto.cmake
>>   | @@ -90,8 +90,40 @@ if (NOT TARGET_OS_DARWIN)
>>   |      endif()
>>   |  endif()
>>   |
>>   | -# gh-3742: investigate LTO warnings.
>>   | -set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} 
>> -Wno-lto-type-mismatch")
>>   | +# {{{ Expose build tools and flags
>>   | +#
>>   | +# It is convenient for building non-cmake targets with the same
>>   | +# flags as we use for sources under CMake control.
>>   | +#
>>   | +# It leans on uncodumented variables that are set in the following
>>   | +# CMake modules: Compiler/GNU.cmake and Compiler/Clang.cmake.
>>   | +
>>   | +# CFLAGS_LTO (list)
>>   | +set(CFLAGS_LTO ${CMAKE_C_COMPILE_OPTIONS_IPO})
>>   | +message(STATUS "CFLAGS_LTO: ${CFLAGS_LTO}")
>>   |
>>   | +# LDFLAGS_LTO (list)
>>   | +set(LDFLAGS_LTO ${CMAKE_C_LINK_OPTIONS_IPO})
>>   | +# FIXME: gh-3742: investigate LTO warnings.
>>   | +list(APPEND LDFLAGS_LTO -Wno-lto-type-mismatch)
>>   | +message(STATUS "LDFLAGS_LTO: ${LDFLAGS_LTO}")
>>   | +
>>   | +# AR_LTO (string)
>>   | +#
>>   | +# Note: Platform/Linux-Intel.cmake and Platform/Windows-MSVC.cmake
>>   | +# set CMAKE_C_CREATE_STATIC_LIBRARY_IPO, but not
>>   | +# CMAKE_C_ARCHIVE_CREATE_IPO. So this snippet is only for GCC and
>>   | +# clang.
>>   | +set(_ar_command ${CMAKE_C_ARCHIVE_CREATE_IPO})
>>   | +separate_arguments(_ar_command)
>>   | +list(GET _ar_command 0 AR_LTO)
>>   | +unset(_ar_command)
>>   | +message(STATUS "AR_LTO: ${AR_LTO}")
>>   | +
>>   | +# }}}
>>   | +
>>   | +# Set build tools and flags for files that are built using CMake.
>>   | +set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} 
>> -Wno-lto-type-mismatch")
>>   |  set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
>>   | +
>>   |  message(STATUS "Enabling LTO: TRUE")
>>
>> Olga, Igor, what do you think about this way?
>>
>> Or, maybe, we should just add LJCORE_O=ljamalg.o to the Make command
>> (just like `make amalg` does)? I was curious and tried it:
>>
>>   | diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>>   | index dd9d748ca..d3df66652 100644
>>   | --- a/cmake/luajit.cmake
>>   | +++ b/cmake/luajit.cmake
>>   | @@ -284,6 +284,7 @@ macro(luajit_build)
>>   |          CCOPT="${luajit_ccopt}"
>>   |          CCDEBUG="${luajit_ccdebug}"
>>   |          XCFLAGS="${luajit_xcflags}"
>>   | +        LJCORE_O=ljamalg.o
>>   |          Q=''
>>   |          # We need to set MACOSX_DEPLOYMENT_TARGET to at least 10.6,
>>   |          # because 10.4 SDK (which is set by default in LuaJIT's
>>
>> And got undefined reference errors from src/lua/utils.c code that use
>> LuaJIT internals.
>>
>> Despite way we'll choose, I would try some benchmarks with jit enabled
>> and disabled (`jit.off()`) to see whether compiling of LuaJIT with LTO
>> gives some gains. Technically it is not part of the task (my bad), so
>> skip it if you're not curious. There is the good candidate to look into:
>> [3].
>>
>> Placed the actual version from the branch [1] to comment it.
>>
>>> commit 62e5d0af7f7a5e44f27b741d43c6c035f20e93b4
>>> Author: Olga Arkhangelskaia <arkholga@tarantool.org>
>>> Date:   Tue Mar 3 12:05:49 2020 +0300
>>>
>>>      cmake: add LTO support for building luajit
>>>           Tarantool has LTO support, however while building luajit 
>>> this opt. was
>>>      omitted. Patch adds necessary flag to turn it on.
>>>           Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 
>>> 2.27+ due
>>>      errors and slow work.
>>>           Closes #3743
>>>
>>> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>>> index 10df633d5..dd9d748ca 100644
>>> --- a/cmake/luajit.cmake
>>> +++ b/cmake/luajit.cmake
>>> @@ -224,6 +224,45 @@ macro(luajit_build)
>>>           set(luajit_xcflags ${luajit_xcflags} -D${def})
>>>       endforeach()
>>>   +    # Add LTO option to luajit
>>> +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
>> I would lean on ENABLE_LTO variable and the fact that all necessary
>> checks already performed by cmake/lto.cmake. I guess it would be more
>> intuitive.
>>
>>> +        message("Setting LTO flags for building luajit")
>> I would use STATUS mode, the message is informational and should not
>> attract user's attention. STATUS mode is used for such messages in
>> cmake/lto.cmake. See [2] for information about message types.
>>
>>> +        # Clang opt to support LTO
>>> +        if (CMAKE_COMPILER_IS_CLANG)
>>> +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
>>> +                set (luajit_cflags ${luajit_cflags} -flto=full)
>>> +            else()
>>> +                # ThinLTO that is both scalable and incremental
>>> +                # due to parallel IPO, available since 3.9 and above.
>>> +                # See 
>>> http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
>>> +                set (luajit_cflags ${luajit_cflags} -flto=thin)
>>> +            endif()
>>> +            # XCode linker dynamically load libLTO.dylib to perform
>>> +            # link-time optimization.
>>> +            # 
>>> http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
>>> +            if (NOT TARGET_OS_DARWIN)
>>> +                # llvm-ar is just a wrapper over ar to pass 
>>> --plugin= option,
>>> +                # so that ar can understand LTO objects.
>>> +                # See 
>>> https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
>>> +                set (CMAKE_AR llvm-ar)
>>> +            endif()
>> I guess it reflects the logic of the following snippet from
>> Compiler/Clang.cmake CMake module:
>>
>>   | string(COMPARE EQUAL "${CMAKE_${lang}_COMPILER_ID}" "AppleClang" 
>> __is_apple_clang)
>>   | <...>
>>   | if(ANDROID OR __is_apple_clang)
>>   |   set(__ar "${CMAKE_AR}")
>>   |   set(__ranlib "${CMAKE_RANLIB}")
>>   | else()
>>   |   set(__ar "${CMAKE_${lang}_COMPILER_AR}")
>>   |   set(__ranlib "${CMAKE_${lang}_COMPILER_RANLIB}")
>>   | endif()
>>   |
>>   | set(CMAKE_${lang}_ARCHIVE_CREATE_IPO
>>   |   "\"${__ar}\" cr <TARGET> <LINK_FLAGS> <OBJECTS>"
>>   | )
>>   |
>>   | set(CMAKE_${lang}_ARCHIVE_APPEND_IPO
>>   |   "\"${__ar}\" r <TARGET> <LINK_FLAGS> <OBJECTS>"
>>   | )
>>   |
>>   | set(CMAKE_${lang}_ARCHIVE_FINISH_IPO
>>   |   "\"${__ranlib}\" <TARGET>"
>>   | )
>>
>> So TARGET_OS_DARWIN check is not accurate: we should check whether Clang
>> is built by Apple or, say, installed from brew; not just OS.
>>
>>> +        else()
>>> +            # GNU opts to support lto
>>> +            # Due to some problems (bugs, slow work, etc) we 
>>> support LTO
>>> +            # only for 5.0+. The same is for binutils prior to 2.27
>>> +            # See comments in scripts/Makefile.lto in scope of
>>> +            # the patch: https://patchwork.kernel.org/patch/10078207/
>>> +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
>>> +                AND NOT ${linker_version} VERSION_LESS 2.27)
>>> +                set (luajit_cflags ${luajit_cflags} -flto 
>>> -fuse-linker-plugin -fno-fat-lto-objects)
>>> +            endif()
>>> +            # gcc-ar is just a wrapper over ar to pass --plugin option
>>> +            # so ar can understand LTO objects. For further info see
>>> +            # -flto and -ffat-lto-objects options descriptions:
>>> +            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
>>> +            set (CMAKE_AR gcc-ar)
>>> +        endif()
>>> +    endif()
>>>       # Pass the same toolchain that is used for building of
>>>       # tarantool itself, because tools from different toolchains
>>>       # can be incompatible. A compiler and a linker are already set
>> [0]: https://gitlab.kitware.com/cmake/cmake/-/issues/16808
>> [1]: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
>> [2]: https://cmake.org/cmake/help/latest/command/message.html
>> [3]: https://gitspartv.github.io/LuaJIT-Benchmarks/
>>
>> WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Igor Munkin]

Sasha,

Again, thanks for such deep research!

On 16.06.20, Alexander Turenko wrote:
> I vote to use the same build tools and flags that CMake uses to build
> files under its control, because otherwise we would write the same logic
> again. It may be not as accurate as one that CMake provides (see example
> below). It may become outdated in a future. And, last but not least,
> duplicated code is painful to maintain.
> 
> However the way I proposed requires to use undocumented CMake variables
> (see [0]). But I think that's evil of the lesser kind.
> 
> I think it would be good to expose related build tool names and flags
> from cmake/lto.cmake and use them in cmake/luajit.cmake. I implemented
> the former part (I would even left STATUS messages as is, they provide
> useful information):
> 
>  | diff --git a/cmake/lto.cmake b/cmake/lto.cmake
>  | index 95ade75f4..79b908e26 100644
>  | --- a/cmake/lto.cmake
>  | +++ b/cmake/lto.cmake
>  | @@ -90,8 +90,40 @@ if (NOT TARGET_OS_DARWIN)
>  |      endif()
>  |  endif()
>  |  
>  | -# gh-3742: investigate LTO warnings.
>  | -set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
>  | +# {{{ Expose build tools and flags
>  | +#
>  | +# It is convenient for building non-cmake targets with the same
>  | +# flags as we use for sources under CMake control.
>  | +#
>  | +# It leans on uncodumented variables that are set in the following
>  | +# CMake modules: Compiler/GNU.cmake and Compiler/Clang.cmake.
>  | +
>  | +# CFLAGS_LTO (list)
>  | +set(CFLAGS_LTO ${CMAKE_C_COMPILE_OPTIONS_IPO})
>  | +message(STATUS "CFLAGS_LTO: ${CFLAGS_LTO}")
>  |  
>  | +# LDFLAGS_LTO (list)
>  | +set(LDFLAGS_LTO ${CMAKE_C_LINK_OPTIONS_IPO})
>  | +# FIXME: gh-3742: investigate LTO warnings.
>  | +list(APPEND LDFLAGS_LTO -Wno-lto-type-mismatch)
>  | +message(STATUS "LDFLAGS_LTO: ${LDFLAGS_LTO}")
>  | +
>  | +# AR_LTO (string)
>  | +#
>  | +# Note: Platform/Linux-Intel.cmake and Platform/Windows-MSVC.cmake
>  | +# set CMAKE_C_CREATE_STATIC_LIBRARY_IPO, but not
>  | +# CMAKE_C_ARCHIVE_CREATE_IPO. So this snippet is only for GCC and
>  | +# clang.
>  | +set(_ar_command ${CMAKE_C_ARCHIVE_CREATE_IPO})
>  | +separate_arguments(_ar_command)
>  | +list(GET _ar_command 0 AR_LTO)
>  | +unset(_ar_command)
>  | +message(STATUS "AR_LTO: ${AR_LTO}")
>  | +
>  | +# }}}
>  | +
>  | +# Set build tools and flags for files that are built using CMake.
>  | +set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-mismatch")
>  |  set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
>  | +
>  |  message(STATUS "Enabling LTO: TRUE")
> 
> Olga, Igor, what do you think about this way?

I'm not such a CMake expert and your patch is a black voodoo magic for
me. I guess Timur is the right person to ask regarding this approach,
considering his CMake experience (Cced him).

> 
> Or, maybe, we should just add LJCORE_O=ljamalg.o to the Make command
> (just like `make amalg` does)? I was curious and tried it:
> 
>  | diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
>  | index dd9d748ca..d3df66652 100644
>  | --- a/cmake/luajit.cmake
>  | +++ b/cmake/luajit.cmake
>  | @@ -284,6 +284,7 @@ macro(luajit_build)
>  |          CCOPT="${luajit_ccopt}"
>  |          CCDEBUG="${luajit_ccdebug}"
>  |          XCFLAGS="${luajit_xcflags}"
>  | +        LJCORE_O=ljamalg.o
>  |          Q=''
>  |          # We need to set MACOSX_DEPLOYMENT_TARGET to at least 10.6,
>  |          # because 10.4 SDK (which is set by default in LuaJIT's
> 
> And got undefined reference errors from src/lua/utils.c code that use
> LuaJIT internals.

Yes, this is a "Poor Man's LTO" (I looked at it sometime ago) and we
discussed this approach offline with Sergey B. I don't undertake to
recommend this build type, due to exact the same reason you faced above:
| $ nm -g ./third_party/luajit/src/libluajit.a | grep -c lj_state_growstack1
| 0

Side note: Heh, this where internal API usage leads to. Hope, we'll fix
it one day.

> 
> Despite way we'll choose, I would try some benchmarks with jit enabled
> and disabled (`jit.off()`) to see whether compiling of LuaJIT with LTO
> gives some gains. Technically it is not part of the task (my bad), so
> skip it if you're not curious. There is the good candidate to look into:
> [3].

I was curious but made no measurement by myself (neither size-related
nor performance-related). I discussed with Sergey offline and we ended
up with the fact this is much closer to compiler testing than to the
patch itself. However, it would be nice to provide some numbers to see
whether this patch has a value.

> 
> Placed the actual version from the branch [1] to comment it.
> 
> > commit 62e5d0af7f7a5e44f27b741d43c6c035f20e93b4
> > Author: Olga Arkhangelskaia <arkholga@tarantool.org>
> > Date:   Tue Mar 3 12:05:49 2020 +0300
> > 
> >     cmake: add LTO support for building luajit
> >     
> >     Tarantool has LTO support, however while building luajit this opt. was
> >     omitted. Patch adds necessary flag to turn it on.
> >     
> >     Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
> >     errors and slow work.
> >     
> >     Closes #3743
> > 
> > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> > index 10df633d5..dd9d748ca 100644
> > --- a/cmake/luajit.cmake
> > +++ b/cmake/luajit.cmake
> > @@ -224,6 +224,45 @@ macro(luajit_build)
> >          set(luajit_xcflags ${luajit_xcflags} -D${def})
> >      endforeach()
> >  
> > +    # Add LTO option to luajit
> > +    if (CMAKE_INTERPROCEDURAL_OPTIMIZATION)
> 
> I would lean on ENABLE_LTO variable and the fact that all necessary
> checks already performed by cmake/lto.cmake. I guess it would be more
> intuitive.
> 
> > +        message("Setting LTO flags for building luajit")
> 
> I would use STATUS mode, the message is informational and should not
> attract user's attention. STATUS mode is used for such messages in
> cmake/lto.cmake. See [2] for information about message types.

Agree.

> 
> > +        # Clang opt to support LTO
> > +        if (CMAKE_COMPILER_IS_CLANG)
> > +            if (${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 3.9)
> > +                set (luajit_cflags ${luajit_cflags} -flto=full)
> > +            else()
> > +                # ThinLTO that is both scalable and incremental
> > +                # due to parallel IPO, available since 3.9 and above.
> > +                # See http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html
> > +                set (luajit_cflags ${luajit_cflags} -flto=thin)
> > +            endif()
> > +            # XCode linker dynamically load libLTO.dylib to perform
> > +            # link-time optimization.
> > +            # http://lists.llvm.org/pipermail/llvm-dev/2009-November/027103.html
> > +            if (NOT TARGET_OS_DARWIN)
> > +                # llvm-ar is just a wrapper over ar to pass --plugin= option,
> > +                # so that ar can understand LTO objects.
> > +                # See https://lists.llvm.org/pipermail/llvm-dev/2018-March/122018.html
> > +                set (CMAKE_AR llvm-ar)
> > +            endif()
> 
> I guess it reflects the logic of the following snippet from
> Compiler/Clang.cmake CMake module:
> 
>  | string(COMPARE EQUAL "${CMAKE_${lang}_COMPILER_ID}" "AppleClang" __is_apple_clang)
>  | <...>
>  | if(ANDROID OR __is_apple_clang)
>  |   set(__ar "${CMAKE_AR}")
>  |   set(__ranlib "${CMAKE_RANLIB}")
>  | else()
>  |   set(__ar "${CMAKE_${lang}_COMPILER_AR}")
>  |   set(__ranlib "${CMAKE_${lang}_COMPILER_RANLIB}")
>  | endif()
>  |
>  | set(CMAKE_${lang}_ARCHIVE_CREATE_IPO
>  |   "\"${__ar}\" cr <TARGET> <LINK_FLAGS> <OBJECTS>"
>  | )
>  |
>  | set(CMAKE_${lang}_ARCHIVE_APPEND_IPO
>  |   "\"${__ar}\" r <TARGET> <LINK_FLAGS> <OBJECTS>"
>  | )
>  |
>  | set(CMAKE_${lang}_ARCHIVE_FINISH_IPO
>  |   "\"${__ranlib}\" <TARGET>"
>  | )
> 
> So TARGET_OS_DARWIN check is not accurate: we should check whether Clang
> is built by Apple or, say, installed from brew; not just OS.

OMG, another fun with MacOS support...

> 
> > +        else()
> > +            # GNU opts to support lto
> > +            # Due to some problems (bugs, slow work, etc) we support LTO
> > +            # only for 5.0+. The same is for binutils prior to 2.27
> > +            # See comments in scripts/Makefile.lto in scope of
> > +            # the patch: https://patchwork.kernel.org/patch/10078207/
> > +            if (NOT ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 5.0
> > +                AND NOT ${linker_version} VERSION_LESS 2.27)
> > +                set (luajit_cflags ${luajit_cflags} -flto -fuse-linker-plugin -fno-fat-lto-objects)
> > +            endif()
> > +            # gcc-ar is just a wrapper over ar to pass --plugin option
> > +            # so ar can understand LTO objects. For further info see
> > +            # -flto and -ffat-lto-objects options descriptions:
> > +            # https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> > +            set (CMAKE_AR gcc-ar)
> > +        endif()
> > +    endif()
> >      # Pass the same toolchain that is used for building of
> >      # tarantool itself, because tools from different toolchains
> >      # can be incompatible. A compiler and a linker are already set
> 
> [0]: https://gitlab.kitware.com/cmake/cmake/-/issues/16808
> [1]: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
> [2]: https://cmake.org/cmake/help/latest/command/message.html
> [3]: https://gitspartv.github.io/LuaJIT-Benchmarks/
> 
> WBR, Alexander Turenko.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Alexander Turenko]

> +    # Enablibg LTO for luajit if DENABLE_LTO set.
> +    if (${ENABLE_LTO})
> +`......message(STATUS "Enablig LTO for luajit")
> +        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})
> +`......set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
> +`......set (CMAKE_AR ${AR_LTO})
> endif()

It seems I forgot to mention one point in the previous message.

Please, set luajit_ar variable and use it below, not reassign CMAKE_AR.
`include(luajit)` directive does not open a new variables scope, so if
we'll reassign CMAKE_AR here, it may affect static libraries creation
for code outside luajit. We should not change outside build rules in
luajit.cmake.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Alexander Turenko]

My review is not sufficient anymore, since I wrote some parts of the
code. Sergey, Timur, can you participate here?

> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
> errors and slow work.

It seems it is not actual anymore (at least for ld). If you want to
highlight minimal requirements, I think it would be better to mention
f9e28ce4602aff3f9bb4e743b0d6167b0f8df88d commit.

Typo: Enablibg.

> +    if (${ENABLE_LTO})

No need to use ${var} inside if condition. See the surrounding code.

> +`......message(STATUS "Enablig LTO for luajit")
> +        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})

The variable is reassigned right below (it is why the build does not
fail).

It seems, you should find a way to verify that your changes actually
enable LTO for luajit. Either by looking into actual compiler / linker
options, or by looking into compiled code differences (say, find a cheap
function in one compilation unit and verify that it is inlined into
another one).

It uses CMAKE_C_LINK_OPTIONS_IPO internal variable directly and ignores
LDFLAGS_LTO that is set by lto.cmake. I suggest to use LDFLAGS_LTO.

I guess you want to set luajit_ldflags, not luajit_ld.

Use spaces for indent in CMake and Lua files. I guess all modern text
editors may be properly configured to have different indent symbol /
level for different file types.

NB: I suggest to rebase your patchsets on top of master on each review
iteration.

----

I know, it is really hard to read the same code again and again when
everything looks finished. But it is inevitable part of any development
process due to the human nature: we do mistakes and should build
processes that allow us to catch them.

I suggest to introduce a personal self-review checklist that will grow
with review comments from teammates.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Timur Safin]

: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Subject: Re: [PATCH] cmake: add LTO support for building luajit
: 
: > +    # Enablibg LTO for luajit if DENABLE_LTO set.
: > +    if (${ENABLE_LTO})
: > +`......message(STATUS "Enablig LTO for luajit")
: > +        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})
: > +`......set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
: > +`......set (CMAKE_AR ${AR_LTO})
: > endif()
: 
: It seems I forgot to mention one point in the previous message.
: 
: Please, set luajit_ar variable and use it below, not reassign CMAKE_AR.
: `include(luajit)` directive does not open a new variables scope, so if
: we'll reassign CMAKE_AR here, it may affect static libraries creation
: for code outside luajit. We should not change outside build rules in
: luajit.cmake.

In general I'd agree with Sasha here, and would advise to introduce scoped variable, (if we were talking about average local variable), but here we are talking about CMAKE_* variable, which is toolchain, and better be used consistently across whole project.

But this might be debatable, though...

Timur

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Timur Safin]

: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Subject: Re: [Tarantool-patches] [PATCH] cmake: add LTO support for
: building luajit
: 
: I vote to use the same build tools and flags that CMake uses to build
: files under its control, because otherwise we would write the same logic
: again. It may be not as accurate as one that CMake provides (see example
: below). It may become outdated in a future. And, last but not least,
: duplicated code is painful to maintain.
: 

I've looked into cmake IPO implementation for different compilers, and essentially they set these 5 variables if asked for IPO:

CMAKE_${lang}_COMPILE_OPTIONS_IPO
CMAKE_${lang}_LINK_OPTIONS_IPO
CMAKE_${lang}_ARCHIVE_CREATE_IPO
CMAKE_${lang}_ARCHIVE_APPEND_IPO
CMAKE_${lang}_ARCHIVE_FINISH_IPO

For C, CXX and ASM as ${lang}

So indeed, propagating those relevant variables down to luajit make variables 
seems adequate approach and flexible enough to all supported by cmake compilers

: I think it would be good to expose related build tool names and flags
: from cmake/lto.cmake and use them in cmake/luajit.cmake. I implemented
: the former part (I would even left STATUS messages as is, they provide
: useful information):
: 
:  | diff --git a/cmake/lto.cmake b/cmake/lto.cmake
:  | index 95ade75f4..79b908e26 100644
:  | --- a/cmake/lto.cmake
:  | +++ b/cmake/lto.cmake
:  | @@ -90,8 +90,40 @@ if (NOT TARGET_OS_DARWIN)
:  |      endif()
:  |  endif()
:  |
:  | -# gh-3742: investigate LTO warnings.
:  | -set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-
: mismatch")
:  | +# {{{ Expose build tools and flags
:  | +#
:  | +# It is convenient for building non-cmake targets with the same
:  | +# flags as we use for sources under CMake control.
:  | +#
:  | +# It leans on uncodumented variables that are set in the following
:  | +# CMake modules: Compiler/GNU.cmake and Compiler/Clang.cmake.
:  | +
:  | +# CFLAGS_LTO (list)
:  | +set(CFLAGS_LTO ${CMAKE_C_COMPILE_OPTIONS_IPO})
:  | +message(STATUS "CFLAGS_LTO: ${CFLAGS_LTO}")
:  |
:  | +# LDFLAGS_LTO (list)
:  | +set(LDFLAGS_LTO ${CMAKE_C_LINK_OPTIONS_IPO})
:  | +# FIXME: gh-3742: investigate LTO warnings.
:  | +list(APPEND LDFLAGS_LTO -Wno-lto-type-mismatch)
:  | +message(STATUS "LDFLAGS_LTO: ${LDFLAGS_LTO}")
:  | +
:  | +# AR_LTO (string)
:  | +#
:  | +# Note: Platform/Linux-Intel.cmake and Platform/Windows-MSVC.cmake
:  | +# set CMAKE_C_CREATE_STATIC_LIBRARY_IPO, but not
:  | +# CMAKE_C_ARCHIVE_CREATE_IPO. So this snippet is only for GCC and
:  | +# clang.
:  | +set(_ar_command ${CMAKE_C_ARCHIVE_CREATE_IPO})
:  | +separate_arguments(_ar_command)
:  | +list(GET _ar_command 0 AR_LTO)
:  | +unset(_ar_command)
:  | +message(STATUS "AR_LTO: ${AR_LTO}")
:  | +
:  | +# }}}
:  | +
:  | +# Set build tools and flags for files that are built using CMake.
:  | +set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wno-lto-type-
: mismatch")
:  |  set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
:  | +
:  |  message(STATUS "Enabling LTO: TRUE")
: 
: Olga, Igor, what do you think about this way?

Looks like this is already applied to the branch, and works the 
as expected.

Here is the theory how to make sure that .o files contain necessary
information for inter-procedure optimizations:

1. For clang it's simple - .o is not ELF file, but rather LLVM IR (bc) file

```
tsafin@M1BOOK6319:~/tarantool/build_lto$ file ./third_party/luajit/src/lj_opt_sink.o
./third_party/luajit/src/lj_opt_sink.o: LLVM IR bitcode
```

we are ok then

2. For gcc it's more complicated - .o file is still ELF file but should
contain GIMPLE IR code in the special sections .gnu.lto.*

```
tsafin@M1BOOK6319:~/tarantool/build_lto$ file  ./third_party/luajit/src/lj_trace.o
./third_party/luajit/src/lj_trace.o: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped

tsafin@M1BOOK6319:~/tarantool/build_lto$ readelf -a ./third_party/luajit/src/lj_trace.o
ELF Header:
  Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
  Class:                             ELF64
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              REL (Relocatable file)
  Machine:                           Advanced Micro Devices X86-64
  Version:                           0x1
  Entry point address:               0x0
  Start of program headers:          0 (bytes into file)
  Start of section headers:          82944 (bytes into file)
  Flags:                             0x0
  Size of this header:               64 (bytes)
  Size of program headers:           0 (bytes)
  Number of program headers:         0
  Size of section headers:           64 (bytes)
  Number of section headers:         48
  Section header string table index: 47

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .text             PROGBITS         0000000000000000  00000040
       0000000000000000  0000000000000000  AX       0     0     1
  [ 2] .data             PROGBITS         0000000000000000  00000040
       0000000000000000  0000000000000000  WA       0     0     1
  [ 3] .bss              NOBITS           0000000000000000  00000040
       0000000000000000  0000000000000000  WA       0     0     1
  [ 4] .gnu.lto_.profile PROGBITS         0000000000000000  00000040
       000000000000000e  0000000000000000   E       0     0     1
  [ 5] .gnu.lto_.icf.bee PROGBITS         0000000000000000  0000004e
       00000000000000ca  0000000000000000   E       0     0     1
  [ 6] .gnu.lto_.jmpfunc PROGBITS         0000000000000000  00000118
       00000000000004fb  0000000000000000   E       0     0     1
  [ 7] .gnu.lto_.inline. PROGBITS         0000000000000000  00000613
       0000000000000524  0000000000000000   E       0     0     1
  [ 8] .gnu.lto_.purecon PROGBITS         0000000000000000  00000b37
       000000000000004b  0000000000000000   E       0     0     1
  [ 9] .gnu.lto_penalty_ PROGBITS         0000000000000000  00000b82
       0000000000000684  0000000000000000   E       0     0     1
  [10] .gnu.lto_trace_fi PROGBITS         0000000000000000  00001206
       0000000000000595  0000000000000000   E       0     0     1
  [11] .gnu.lto_trace_sa PROGBITS         0000000000000000  0000179b
       0000000000000611  0000000000000000   E       0     0     1
  [12] .gnu.lto_trace_st PROGBITS         0000000000000000  00001dac
       0000000000000d49  0000000000000000   E       0     0     1
  [13] .gnu.lto_trace_ex PROGBITS         0000000000000000  00002af5
       0000000000000255  0000000000000000   E       0     0     1
  [14] .gnu.lto_trace_un PROGBITS         0000000000000000  00002d4a
       00000000000004f9  0000000000000000   E       0     0     1
  [15] .gnu.lto_trace_fl PROGBITS         0000000000000000  00003243
       000000000000050c  0000000000000000   E       0     0     1
  [16] .gnu.lto_trace_ex PROGBITS         0000000000000000  0000374f
       00000000000004cc  0000000000000000   E       0     0     1
  [17] .gnu.lto_lj_trace PROGBITS         0000000000000000  00003c1b
       000000000000031f  0000000000000000   E       0     0     1
  [18] .gnu.lto_lj_trace PROGBITS         0000000000000000  00003f3a
       0000000000000291  0000000000000000   E       0     0     1
  [19] .gnu.lto_lj_trace PROGBITS         0000000000000000  000041cb
       000000000000024a  0000000000000000   E       0     0     1
  [20] .gnu.lto_lj_trace PROGBITS         0000000000000000  00004415
       000000000000047e  0000000000000000   E       0     0     1
  [21] .gnu.lto_lj_trace PROGBITS         0000000000000000  00004893
       0000000000000429  0000000000000000   E       0     0     1
  [22] .gnu.lto_lj_trace PROGBITS         0000000000000000  00004cbc
       00000000000004a7  0000000000000000   E       0     0     1
  [23] .gnu.lto_lj_trace PROGBITS         0000000000000000  00005163
       00000000000001f3  0000000000000000   E       0     0     1
  [24] .gnu.lto_lj_trace PROGBITS         0000000000000000  00005356
       00000000000002a9  0000000000000000   E       0     0     1
  [25] .gnu.lto_lj_trace PROGBITS         0000000000000000  000055ff
       0000000000000840  0000000000000000   E       0     0     1
  [26] .gnu.lto_trace_st PROGBITS         0000000000000000  00005e3f
       0000000000000be5  0000000000000000   E       0     0     1
  [27] .gnu.lto_trace_st PROGBITS         0000000000000000  00006a24
       000000000000030c  0000000000000000   E       0     0     1
  [28] .gnu.lto_trace_ab PROGBITS         0000000000000000  00006d30
       0000000000001335  0000000000000000   E       0     0     1
  [29] .gnu.lto_trace_st PROGBITS         0000000000000000  00008065
       0000000000000f5b  0000000000000000   E       0     0     1
  [30] .gnu.lto_lj_trace PROGBITS         0000000000000000  00008fc0
       0000000000000387  0000000000000000   E       0     0     1
  [31] .gnu.lto_lj_trace PROGBITS         0000000000000000  00009347
       000000000000055c  0000000000000000   E       0     0     1
  [32] .gnu.lto_lj_trace PROGBITS         0000000000000000  000098a3
       00000000000003ed  0000000000000000   E       0     0     1
  [33] .gnu.lto_trace_ho PROGBITS         0000000000000000  00009c90
       000000000000051c  0000000000000000   E       0     0     1
  [34] .gnu.lto_lj_trace PROGBITS         0000000000000000  0000a1ac
       00000000000001ef  0000000000000000   E       0     0     1
  [35] .gnu.lto_lj_trace PROGBITS         0000000000000000  0000a39b
       000000000000041c  0000000000000000   E       0     0     1
  [36] .gnu.lto_lj_trace PROGBITS         0000000000000000  0000a7b7
       0000000000000251  0000000000000000   E       0     0     1
  [37] .gnu.lto_lj_trace PROGBITS         0000000000000000  0000aa08
       0000000000000f22  0000000000000000   E       0     0     1
  [38] .gnu.lto_.symbol_ PROGBITS         0000000000000000  0000b92a
       00000000000003cb  0000000000000000   E       0     0     1
  [39] .gnu.lto_.refs.be PROGBITS         0000000000000000  0000bcf5
       000000000000001b  0000000000000000   E       0     0     1
  [40] .gnu.lto_.decls.b PROGBITS         0000000000000000  0000bd10
       000000000000769a  0000000000000000   E       0     0     1
  [41] .gnu.lto_.symtab. PROGBITS         0000000000000000  000133aa
       0000000000000465  0000000000000000   E       0     0     1
  [42] .gnu.lto_.opts    PROGBITS         0000000000000000  0001380f
       00000000000000d1  0000000000000000   E       0     0     1

...
```


So it looks LGTM at the moment (once been rebased)

Timur

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Timur Safin]

: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Behalf Of Timur Safin
: Subject: Re: [Tarantool-patches] [PATCH] cmake: add LTO support for
: building luajit
: 
...
: Here is the theory how to make sure that .o files contain necessary
: information for inter-procedure optimizations:
: 

Forgot to include whole logs:
* gcc - https://pastebin.com/raw/u59grhLe
* clang - https://pastebin.com/raw/pabDE1B2

Regards,
Timur

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

Hi!

Thank you guys for the deep review and your patience.

I have fixed issues that you have underlined.

Added messages with options in case DENABLE_LTO=ON in order to see all 
flags that we enabling.

Checked with file/readelf and updated branch. So the patch(luajit part) 
looks like:

   diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
   index 10df633d5..555bc8371 100644
   --- a/cmake/luajit.cmake
   +++ b/cmake/luajit.cmake
   @@ -230,6 +230,16 @@ macro(luajit_build)
        # above.
        set (luajit_ld ${CMAKE_LINKER})
        set (luajit_ar ${CMAKE_AR} rcus)
   +    # Enablibg LTO for luajit if DENABLE_LTO set.
   +    if (ENABLE_LTO)
   +        message(STATUS "Enable LTO for luajit")
   +        set (luajit_ldflags ${luajit_ldflags} ${LDFLAGS_LTO})
   +        message(STATUS "ld: " ${luajit_ldflags})
   +        set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
   +        message(STATUS "cflags: " ${luajit_cflags})
   +        set (luajit_ar  ${AR_LTO} rcus)
   +        message(STATUS "ar: " ${luajit_ar})
   + endif()
        set (luajit_strip ${CMAKE_STRIP})
~


16.06.2020 21:31, Alexander Turenko пишет:
> My review is not sufficient anymore, since I wrote some parts of the
> code. Sergey, Timur, can you participate here?
>
>> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due
>> errors and slow work.
> It seems it is not actual anymore (at least for ld). If you want to
> highlight minimal requirements, I think it would be better to mention
> f9e28ce4602aff3f9bb4e743b0d6167b0f8df88d commit.
>
> Typo: Enablibg.
>
>> +    if (${ENABLE_LTO})
> No need to use ${var} inside if condition. See the surrounding code.
>
>> +`......message(STATUS "Enablig LTO for luajit")
>> +        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO})
> The variable is reassigned right below (it is why the build does not
> fail).
>
> It seems, you should find a way to verify that your changes actually
> enable LTO for luajit. Either by looking into actual compiler / linker
> options, or by looking into compiled code differences (say, find a cheap
> function in one compilation unit and verify that it is inlined into
> another one).
>
> It uses CMAKE_C_LINK_OPTIONS_IPO internal variable directly and ignores
> LDFLAGS_LTO that is set by lto.cmake. I suggest to use LDFLAGS_LTO.
>
> I guess you want to set luajit_ldflags, not luajit_ld.
>
> Use spaces for indent in CMake and Lua files. I guess all modern text
> editors may be properly configured to have different indent symbol /
> level for different file types.
>
> NB: I suggest to rebase your patchsets on top of master on each review
> iteration.
>
> ----
>
> I know, it is really hard to read the same code again and again when
> everything looks finished. But it is inevitable part of any development
> process due to the human nature: we do mistakes and should build
> processes that allow us to catch them.
>
> I suggest to introduce a personal self-review checklist that will grow
> with review comments from teammates.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Alexander Turenko]

LGTM.

There are several style comments: feel free to fix them or ignore. No
need to re-review with me.

WBR, Alexander Turenko.

> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci

Pasted parts of actual patch comment it.

> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 10df633d5..555bc8371 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -230,6 +230,16 @@ macro(luajit_build)
>      # above.
>      set (luajit_ld ${CMAKE_LINKER})
>      set (luajit_ar ${CMAKE_AR} rcus)
> +    # Enablibg LTO for luajit if DENABLE_LTO set.

Typo: Enablibg.

Nit: I would move it below `set (luajit_strip ${CMAKE_STRIP})` line and
format it as a separate block (add an empty line before it). Just for
readability.

> +    if (ENABLE_LTO)
> +        message(STATUS "Enable LTO for luajit")
> +        set (luajit_ldflags ${luajit_ldflags} ${LDFLAGS_LTO})
> +        message(STATUS "ld: " ${luajit_ldflags})
> +        set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
> +        message(STATUS "cflags: " ${luajit_cflags})
> +        set (luajit_ar  ${AR_LTO} rcus)
> +        message(STATUS "ar: " ${luajit_ar})
> +    endif()

Nit: Those variables are lists and they are printed without spaces
(I don't know exact reason):

 | -- Enable LTO for luajit
 | -- ld: -Wno-lto-type-mismatch-Wno-lto-type-mismatch
 | -- cflags: -Wno-unknown-pragmas-fexceptions-funwind-tables-fno-omit-frame-pointer-fno-stack-protector-fno-common-msse2-Wno-parentheses-equality-Wno-tautological-compare-Wno-varargs-Wno-implicit-fallthrough-flto=thin
 | -- ar: "/usr/lib/llvm-8/bin/llvm-ar"rcus

You should include them into quotes, like so:

 | -message(STATUS "ld: " ${luajit_ldflags})
 | +message(STATUS "ld: ${luajit_ldflags}")

(BTW, it is not ld executable, but ldflags; so I would name it 'ldflags'
in the output.)

Nit: LTO flags are already printed before:

 | -- ld version string: GNU ld (GNU Binutils for Debian) 2.31.1
 | -- Found ld.bfd version: 2.31
 | -- CFLAGS_LTO: -flto=thin
 | -- LDFLAGS_LTO: -Wno-lto-type-mismatch
 | -- AR_LTO: "/usr/lib/llvm-8/bin/llvm-ar"
 | -- Enabling LTO: TRUE

But I don't have strict objection against printing it again.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Kirill Yukhin]

Hello,

On 12 мар 13:05, Olga Arkhangelskaia wrote:
> Tarantool has LTO support, however while building luajit this opt. was
> omitted. Patch adds necessary flag to turn it on.
> 
> Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> 
> Closes #3743
> ---
> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci

I've checked your patch into 2.4 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Alexander Turenko]

BTW, just found that we don't test LTO on Mac OS X :(

https://gitlab.com/tarantool/tarantool/-/jobs/627735445

jobs name: osx_15_release_lto

From logs:

> -- Enabling LTO: FALSE

Olga, do you test LTO on Mac OS X manually? If so, it is not blocker.

Alexander will look at the problem with CI (CCed).

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Kirill Yukhin]

On 08 июл 15:34, Kirill Yukhin wrote:
> Hello,
> 
> On 12 мар 13:05, Olga Arkhangelskaia wrote:
> > Tarantool has LTO support, however while building luajit this opt. was
> > omitted. Patch adds necessary flag to turn it on.
> > 
> > Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+.
> > 
> > Closes #3743
> > ---
> > Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci
> 
> I've checked your patch into 2.4 and master.

Since this is a feature, I've reverted it from 2.4

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Olga Arkhangelskaia]

08.07.2020 15:38, Alexander Turenko пишет:
> BTW, just found that we don't test LTO on Mac OS X :(
>
> https://gitlab.com/tarantool/tarantool/-/jobs/627735445
>
> jobs name: osx_15_release_lto
>
>  From logs:
>
>> -- Enabling LTO: FALSE
> Olga, do you test LTO on Mac OS X manually? If so, it is not blocker.

Hey! I have tested LTO in Mojave 10.14.1 (my macbook).

With DENABLE_LTO=ON

cflags: 
-Wno-unknown-pragmas-fexceptions-funwind-tables-fno-omit-frame-pointer-fno-stack-protector-fno-common-msse2-Wno-parentheses-equality-Wno-tautological-compare-Wno-varargs-Wno-implicit-fallthrough-isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk-flto=thin

third_party/luajit/src/lib_aux.o: LLVM bitcode, wrapper x86_64

Without DENABLE_LTO=ON

third_party/luajit/src/lib_aux.o: Mach-O 64-bit object x86_64

>
> Alexander will look at the problem with CI (CCed).
>
> WBR, Alexander Turenko.