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

[Igor Munkin <imun@tarantool.org>]

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