Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add LTO cache, customize LTO settings for tests #1513

Merged
merged 6 commits into from
May 11, 2022
Merged

Add LTO cache, customize LTO settings for tests #1513

merged 6 commits into from
May 11, 2022

Conversation

simone-gaia
Copy link
Contributor

LaurentiuCristofor
LaurentiuCristofor reviewed May 10, 2022
View reviewed changes
production/cmake/gaia_internal.cmake Outdated

# Enable Thin LTO only on non-test targets.
if(ENABLE_LTO)
if (IS_TEST)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove space after if.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also on line 38 above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

LaurentiuCristofor
LaurentiuCristofor reviewed May 10, 2022
View reviewed changes
production/cmake/gaia_internal.cmake Outdated
set(IS_TEST true)
endif()

# TODO: this is currently useless because LLD perform LTO on tests even with -fno-lto
Copy link
Contributor

@LaurentiuCristofor LaurentiuCristofor May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this comment be placed inside the if (IS_TEST) block below? Otherwise, it's confusing what "this" is supposed to refer to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

LaurentiuCristofor
LaurentiuCristofor approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@LaurentiuCristofor LaurentiuCristofor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some notes on small issues.

@senderista
Copy link
Contributor

So I wonder if the problem with -fno-lto not working is that configure_gaia_target() is called before target_link_libraries(), so that the default -flto=thin compile/link option for Gaia static libs is appended to the compiler/linker command line after -fno-lto? If so, then you should be able to fix it by just calling configure_gaia_target() after target_link_libraries(). (I think this is an example of static lib targets being a leaky abstraction: specifying PRIVATE for a static lib property doesn't mean that dependent targets won't get options for the static lib on their own compiler/linker command line, because those options aren't applied until the static lib is actually linked by a dependent target.)

Did you verify the order of -flto=thin vs. -fno-lto for the test targets' compiler/linker commands?

@simone-gaia
Copy link
Contributor Author

So I wonder if the problem with -fno-lto not working is that configure_gaia_target() is called before target_link_libraries(), so that the default -flto=thin compile/link option for Gaia static libs is appended to the compiler/linker command line after -fno-lto? If so, then you should be able to fix it by just calling configure_gaia_target() after target_link_libraries(). (I think this is an example of static lib targets being a leaky abstraction: specifying PRIVATE for a static lib property doesn't mean that dependent targets won't get options for the static lib on their own compiler/linker command line, because those options aren't applied until the static lib is actually linked by a dependent target.)

Did you verify the order of -flto=thin vs. -fno-lto for the test targets' compiler/linker commands?

The test compile/link commands do not have the lto setting at all, besides -fno-lto param.

@senderista
Copy link
Contributor

Also, have you looked at using the INTERPROCEDURAL_OPTIMIZATION property instead of -flto, or is that incapable of specifying -flto=thin?

@senderista
Copy link
Contributor

Also, have you looked at using the INTERPROCEDURAL_OPTIMIZATION property instead of -flto, or is that incapable of specifying -flto=thin?

Hmm, looks like INTERPROCEDURAL_OPTIMIZATION=ON sets -flto=thin by default: https://android.googlesource.com/platform/external/cmake/+/upstream-release/Modules/Compiler/Clang.cmake

@senderista
Copy link
Contributor

Oh, one more thing I forgot to ask before: shouldn't we enable LTO only in release builds? At least I can't think of any reason to enable it for debug builds...

@simone-gaia
Copy link
Contributor Author

@senderista

Also, have you looked at using the INTERPROCEDURAL_OPTIMIZATION property instead of -flto, or is that incapable of specifying -flto=thin?

Just tried and seems to be the same as -flto=thin.

Oh, one more thing I forgot to ask before: shouldn't we enable LTO only in release builds? At least I can't think of any reason to enable it for debug builds...

Done.

senderista
senderista reviewed May 10, 2022
View reviewed changes
production/CMakeLists.txt
@@ -48,6 +48,14 @@ if (NOT ENABLE_CLANG_TIDY)
endif()
endif()

if (NOT DEFINED ENABLE_LTO)
if(CMAKE_BUILD_TYPE STREQUAL "Debug")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads strangely. Why would clang-tidy settings ever be dependent on LTO settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

senderista
senderista approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@senderista senderista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, as long as disabling LTO behaves as expected. If not, we can just leave LTO off by default and use it only for SDK builds or perf tests (possibly combining ENABLE_LTO=ON with DISABLE_ASSERTS=ON).

@daxhaw
Copy link
Contributor

daxhaw commented May 10, 2022

Not much to add except if you want to go ahead and make the change for building the SDK with LTO I suggest adding the following:

  1. add a new build-type named ReleaseLTO
  2. This gets plumbed through in the build-job actions.yml for the Release SDK builds
  3. Add support for a cfg-enable in gdev for ReleaseLTO.
  4. add this build-type in the main.yml SDK build job.

Or something similar to that.

@simone-gaia
Copy link
Contributor Author

Not much to add except if you want to go ahead and make the change for building the SDK with LTO I suggest adding the following:

  1. add a new build-type named ReleaseLTO
  2. This gets plumbed through in the build-job actions.yml for the Release SDK builds
  3. Add support for a cfg-enable in gdev for ReleaseLTO.
  4. add this build-type in the main.yml SDK build job.

Or something similar to that.

Will do it in a separate PR. Thanks for the tip.

@simone-gaia simone-gaia merged commit 3954b56 into master May 11, 2022
@simone-gaia simone-gaia deleted the rondelli-lto-cache branch May 11, 2022 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@senderista senderista senderista approved these changes

@LaurentiuCristofor LaurentiuCristofor LaurentiuCristofor approved these changes

@daxhaw daxhaw Awaiting requested review from daxhaw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@simone-gaia @senderista @daxhaw @LaurentiuCristofor