From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C84634696C3 for ; Tue, 14 Apr 2020 13:00:00 +0300 (MSK) References: <20200312100549.31608-1-arkholga@tarantool.org> <20200414091803.GJ5713@tarantool.org> From: Olga Arkhangelskaia Message-ID: <53a91575-b340-1c21-e0f2-1f894bd8e7c5@tarantool.org> Date: Tue, 14 Apr 2020 12:59:53 +0300 MIME-Version: 1.0 In-Reply-To: <20200414091803.GJ5713@tarantool.org> Content-Type: multipart/alternative; boundary="------------879D8237A746A5FB20994D2A" Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org This is a multi-part message in MIME format. --------------879D8237A746A5FB20994D2A Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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) > >> + 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) >> --------------879D8237A746A5FB20994D2A Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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)




+	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)



    

  


--------------879D8237A746A5FB20994D2A--