* [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit
@ 2020-03-12 10:05 Olga Arkhangelskaia
2020-03-12 10:10 ` Cyrill Gorcunov
` (5 more replies)
0 siblings, 6 replies; 33+ messages in thread
From: Olga Arkhangelskaia @ 2020-03-12 10:05 UTC (permalink / raw)
To: tarantool-patches
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)
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-03-12 10:05 [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit Olga Arkhangelskaia @ 2020-03-12 10:10 ` Cyrill Gorcunov 2020-04-14 9:18 ` Igor Munkin ` (4 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Cyrill Gorcunov @ 2020-03-12 10:10 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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> ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-03-12 10:05 [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit Olga Arkhangelskaia 2020-03-12 10:10 ` Cyrill Gorcunov @ 2020-04-14 9:18 ` Igor Munkin 2020-04-14 9:59 ` Olga Arkhangelskaia ` (2 more replies) 2020-06-16 1:02 ` Alexander Turenko ` (3 subsequent siblings) 5 siblings, 3 replies; 33+ messages in thread From: Igor Munkin @ 2020-04-14 9:18 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-14 9:18 ` Igor Munkin @ 2020-04-14 9:59 ` Olga Arkhangelskaia 2020-04-14 12:32 ` Olga Arkhangelskaia 2020-04-14 14:33 ` Olga Arkhangelskaia 2 siblings, 0 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-04-14 9:59 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-14 9:18 ` Igor Munkin 2020-04-14 9:59 ` Olga Arkhangelskaia @ 2020-04-14 12:32 ` Olga Arkhangelskaia 2020-04-14 17:00 ` Igor Munkin 2020-04-14 14:33 ` Olga Arkhangelskaia 2 siblings, 1 reply; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-04-14 12:32 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches 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) >> ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-14 12:32 ` Olga Arkhangelskaia @ 2020-04-14 17:00 ` Igor Munkin 2020-04-15 13:22 ` Olga Arkhangelskaia 0 siblings, 1 reply; 33+ messages in thread From: Igor Munkin @ 2020-04-14 17:00 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-14 17:00 ` Igor Munkin @ 2020-04-15 13:22 ` Olga Arkhangelskaia 2020-04-17 11:47 ` Igor Munkin 0 siblings, 1 reply; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-04-15 13:22 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-15 13:22 ` Olga Arkhangelskaia @ 2020-04-17 11:47 ` Igor Munkin 2020-04-17 14:41 ` Olga Arkhangelskaia 0 siblings, 1 reply; 33+ messages in thread From: Igor Munkin @ 2020-04-17 11:47 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-17 11:47 ` Igor Munkin @ 2020-04-17 14:41 ` Olga Arkhangelskaia 2020-04-27 23:04 ` Igor Munkin 0 siblings, 1 reply; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-04-17 14:41 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches 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() ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-17 14:41 ` Olga Arkhangelskaia @ 2020-04-27 23:04 ` Igor Munkin 2020-05-06 10:47 ` Olga Arkhangelskaia 0 siblings, 1 reply; 33+ messages in thread From: Igor Munkin @ 2020-04-27 23:04 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-27 23:04 ` Igor Munkin @ 2020-05-06 10:47 ` Olga Arkhangelskaia 0 siblings, 0 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-05-06 10:47 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-14 9:18 ` Igor Munkin 2020-04-14 9:59 ` Olga Arkhangelskaia 2020-04-14 12:32 ` Olga Arkhangelskaia @ 2020-04-14 14:33 ` Olga Arkhangelskaia 2020-05-25 12:58 ` Sergey Bronnikov 2 siblings, 1 reply; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-04-14 14:33 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches 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) >> ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-04-14 14:33 ` Olga Arkhangelskaia @ 2020-05-25 12:58 ` Sergey Bronnikov 2020-05-25 15:00 ` Olga Arkhangelskaia 2020-05-25 15:12 ` Olga Arkhangelskaia 0 siblings, 2 replies; 33+ messages in thread From: Sergey Bronnikov @ 2020-05-25 12:58 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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) > > > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-05-25 12:58 ` Sergey Bronnikov @ 2020-05-25 15:00 ` Olga Arkhangelskaia 2020-05-25 15:12 ` Olga Arkhangelskaia 1 sibling, 0 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-05-25 15:00 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches 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) >>>> ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-05-25 12:58 ` Sergey Bronnikov 2020-05-25 15:00 ` Olga Arkhangelskaia @ 2020-05-25 15:12 ` Olga Arkhangelskaia 2020-05-25 15:43 ` Sergey Bronnikov 2020-05-26 10:11 ` Igor Munkin 1 sibling, 2 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-05-25 15:12 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches 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) >>>> ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-05-25 15:12 ` Olga Arkhangelskaia @ 2020-05-25 15:43 ` Sergey Bronnikov 2020-05-26 10:11 ` Igor Munkin 1 sibling, 0 replies; 33+ messages in thread From: Sergey Bronnikov @ 2020-05-25 15:43 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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@ ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-05-25 15:12 ` Olga Arkhangelskaia 2020-05-25 15:43 ` Sergey Bronnikov @ 2020-05-26 10:11 ` Igor Munkin 2020-05-27 10:01 ` Olga Arkhangelskaia 1 sibling, 1 reply; 33+ messages in thread From: Igor Munkin @ 2020-05-26 10:11 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches, Alexander Turenko 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-05-26 10:11 ` Igor Munkin @ 2020-05-27 10:01 ` Olga Arkhangelskaia 0 siblings, 0 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-05-27 10:01 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Alexander Turenko [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-03-12 10:05 [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit Olga Arkhangelskaia 2020-03-12 10:10 ` Cyrill Gorcunov 2020-04-14 9:18 ` Igor Munkin @ 2020-06-16 1:02 ` Alexander Turenko 2020-06-16 11:36 ` Olga Arkhangelskaia ` (2 more replies) 2020-07-08 12:23 ` Alexander Turenko ` (2 subsequent siblings) 5 siblings, 3 replies; 33+ messages in thread From: Alexander Turenko @ 2020-06-16 1:02 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 1:02 ` Alexander Turenko @ 2020-06-16 11:36 ` Olga Arkhangelskaia 2020-06-16 12:01 ` Olga Arkhangelskaia ` (2 more replies) 2020-06-16 12:53 ` Igor Munkin 2020-06-25 9:45 ` Timur Safin 2 siblings, 3 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-06-16 11:36 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 11:36 ` Olga Arkhangelskaia @ 2020-06-16 12:01 ` Olga Arkhangelskaia 2020-06-16 17:34 ` Alexander Turenko 2020-06-16 18:31 ` Alexander Turenko 2 siblings, 0 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-06-16 12:01 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 11:36 ` Olga Arkhangelskaia 2020-06-16 12:01 ` Olga Arkhangelskaia @ 2020-06-16 17:34 ` Alexander Turenko 2020-06-25 9:19 ` Timur Safin 2020-06-16 18:31 ` Alexander Turenko 2 siblings, 1 reply; 33+ messages in thread From: Alexander Turenko @ 2020-06-16 17:34 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches > + # 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 17:34 ` Alexander Turenko @ 2020-06-25 9:19 ` Timur Safin 0 siblings, 0 replies; 33+ messages in thread From: Timur Safin @ 2020-06-25 9:19 UTC (permalink / raw) To: 'Alexander Turenko', 'Olga Arkhangelskaia' Cc: tarantool-patches : 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 11:36 ` Olga Arkhangelskaia 2020-06-16 12:01 ` Olga Arkhangelskaia 2020-06-16 17:34 ` Alexander Turenko @ 2020-06-16 18:31 ` Alexander Turenko 2020-06-29 20:16 ` Olga Arkhangelskaia 2 siblings, 1 reply; 33+ messages in thread From: Alexander Turenko @ 2020-06-16 18:31 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 18:31 ` Alexander Turenko @ 2020-06-29 20:16 ` Olga Arkhangelskaia 0 siblings, 0 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-06-29 20:16 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 1:02 ` Alexander Turenko 2020-06-16 11:36 ` Olga Arkhangelskaia @ 2020-06-16 12:53 ` Igor Munkin 2020-06-25 9:45 ` Timur Safin 2 siblings, 0 replies; 33+ messages in thread From: Igor Munkin @ 2020-06-16 12:53 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-16 1:02 ` Alexander Turenko 2020-06-16 11:36 ` Olga Arkhangelskaia 2020-06-16 12:53 ` Igor Munkin @ 2020-06-25 9:45 ` Timur Safin 2020-06-25 9:47 ` Timur Safin 2 siblings, 1 reply; 33+ messages in thread From: Timur Safin @ 2020-06-25 9:45 UTC (permalink / raw) To: 'Alexander Turenko', 'Olga Arkhangelskaia' Cc: tarantool-patches : 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-06-25 9:45 ` Timur Safin @ 2020-06-25 9:47 ` Timur Safin 0 siblings, 0 replies; 33+ messages in thread From: Timur Safin @ 2020-06-25 9:47 UTC (permalink / raw) To: 'Alexander Turenko', 'Olga Arkhangelskaia' Cc: tarantool-patches : 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-03-12 10:05 [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit Olga Arkhangelskaia ` (2 preceding siblings ...) 2020-06-16 1:02 ` Alexander Turenko @ 2020-07-08 12:23 ` Alexander Turenko 2020-07-08 12:34 ` Kirill Yukhin 2020-07-08 12:38 ` Alexander Turenko 5 siblings, 0 replies; 33+ messages in thread From: Alexander Turenko @ 2020-07-08 12:23 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-03-12 10:05 [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit Olga Arkhangelskaia ` (3 preceding siblings ...) 2020-07-08 12:23 ` Alexander Turenko @ 2020-07-08 12:34 ` Kirill Yukhin 2020-07-08 12:42 ` Kirill Yukhin 2020-07-08 12:38 ` Alexander Turenko 5 siblings, 1 reply; 33+ messages in thread From: Kirill Yukhin @ 2020-07-08 12:34 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-07-08 12:34 ` Kirill Yukhin @ 2020-07-08 12:42 ` Kirill Yukhin 0 siblings, 0 replies; 33+ messages in thread From: Kirill Yukhin @ 2020-07-08 12:42 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-03-12 10:05 [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit Olga Arkhangelskaia ` (4 preceding siblings ...) 2020-07-08 12:34 ` Kirill Yukhin @ 2020-07-08 12:38 ` Alexander Turenko 2020-07-09 5:13 ` Olga Arkhangelskaia 5 siblings, 1 reply; 33+ messages in thread From: Alexander Turenko @ 2020-07-08 12:38 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit 2020-07-08 12:38 ` Alexander Turenko @ 2020-07-09 5:13 ` Olga Arkhangelskaia 0 siblings, 0 replies; 33+ messages in thread From: Olga Arkhangelskaia @ 2020-07-09 5:13 UTC (permalink / raw) To: Alexander Turenko; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2020-07-09 5:13 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-12 10:05 [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit Olga Arkhangelskaia 2020-03-12 10:10 ` Cyrill Gorcunov 2020-04-14 9:18 ` Igor Munkin 2020-04-14 9:59 ` Olga Arkhangelskaia 2020-04-14 12:32 ` Olga Arkhangelskaia 2020-04-14 17:00 ` Igor Munkin 2020-04-15 13:22 ` Olga Arkhangelskaia 2020-04-17 11:47 ` Igor Munkin 2020-04-17 14:41 ` Olga Arkhangelskaia 2020-04-27 23:04 ` Igor Munkin 2020-05-06 10:47 ` Olga Arkhangelskaia 2020-04-14 14:33 ` Olga Arkhangelskaia 2020-05-25 12:58 ` Sergey Bronnikov 2020-05-25 15:00 ` Olga Arkhangelskaia 2020-05-25 15:12 ` Olga Arkhangelskaia 2020-05-25 15:43 ` Sergey Bronnikov 2020-05-26 10:11 ` Igor Munkin 2020-05-27 10:01 ` Olga Arkhangelskaia 2020-06-16 1:02 ` Alexander Turenko 2020-06-16 11:36 ` Olga Arkhangelskaia 2020-06-16 12:01 ` Olga Arkhangelskaia 2020-06-16 17:34 ` Alexander Turenko 2020-06-25 9:19 ` Timur Safin 2020-06-16 18:31 ` Alexander Turenko 2020-06-29 20:16 ` Olga Arkhangelskaia 2020-06-16 12:53 ` Igor Munkin 2020-06-25 9:45 ` Timur Safin 2020-06-25 9:47 ` Timur Safin 2020-07-08 12:23 ` Alexander Turenko 2020-07-08 12:34 ` Kirill Yukhin 2020-07-08 12:42 ` Kirill Yukhin 2020-07-08 12:38 ` Alexander Turenko 2020-07-09 5:13 ` Olga Arkhangelskaia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox