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.

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

cmake: Add LTO functionality #52

Closed
wants to merge 50 commits into from
Closed

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Nov 15, 2023

The LTO_FLAGS CMake variable might be used to override CMake's default LTO flags.

The following build system configuration:

cmake -B build -DLTO=ON -DLTO_FLAGS="-flto"

is comparable with the master branch amended with bitcoin#28876.

@hebasto
Copy link
Owner Author

hebasto commented Nov 16, 2023

Rebased.

@fanquake
Copy link

Repeating my comments from bitcoin#28876. I'm not a fan of this interface. If we are going to have a dedicated LTO option, it shouldn't also require manually passing flags into non-standard variables.

It's also not clear to me why:

cmake -B build -DLTO=ON -DLTO_FLAGS="-flto"

is any better than just:

cmake -B build CXX_FLAGS="-flto"

In this branch, is LTO_FLAGS meant to be for any flags, i.e compile and link?

@hebasto
Copy link
Owner Author

hebasto commented Nov 16, 2023

It's also not clear to me why:

cmake -B build -DLTO=ON -DLTO_FLAGS="-flto"

is any better than just:

cmake -B build CXX_FLAGS="-flto"

In this branch, is LTO_FLAGS meant to be for any flags, i.e compile and link?

  1. In this branch, the LTO_FLAGS is meant to be for flags that are supposed to be passed to a compiler driver for any language (C or C++) in any mode (compiling or linking).

  2. The default CMake LTO flags are better for everyday developer builds than just -flto for both GCC and Clang. They need to be adjusted only for release builds to be on-par with the master branch.

  3. The LTO CMake variable was chosen to accomplish feature parity requirement for a new build system. Agree with you that LTO_FLAGS looks ugly.

  4. Both ways:

CXXFLAGS="-flto" cmake -B build

and

cmake -B build -DCMAKE_CXX_FLAGS="-flto"

have the common drawback, which is no support for C code. That means that all object files in the build/CMakeFiles/secp256k1.dir/src/secp256k1/src are compiled without LTO flags.


I'm not a fan of this interface.

Happy to hear other interface ideas / prototypes :)

@fanquake
Copy link

fanquake commented Nov 16, 2023

Right. So we've still got to use LDFLAGS to pass through normal LTO link options, with your current approach? So why not just keep using the other standard *FLAGS vars? Otherwise now we've also got to mix and match.

What are the default CMake LTO flags? Why are they better? Are they stable (not changing from release to release)?

which is no support for C code.

Why wouldn't it support C code; that's what CFLAGS is for? I only omitted it here to simplify examples.

@hebasto
Copy link
Owner Author

hebasto commented Nov 16, 2023

What are the default CMake LTO flags? Why are they better?

For GCC:

-flto=auto -fno-fat-lto-objects

with the main benefit of parallelizing of LTRANS jobs, which in turn means:

  • quicker build
  • no "lto-wrapper: warning: using serial compilation of 128 LTRANS jobs" warnings

For Clang:

-flto-thin

with benefits as follows:

  • quicker build
  • compatibility with OSS-Fuzz

Are they stable (not changing from release to release)?

No, they are not.


If I understand your point correctly, this PR could be closed and all needed LTO flags are to be provided via standard variables, right?

@fanquake
Copy link

I think the PR should stay open for further discussion/other ideas, but yes, my main point is that we shouldn't be creating a special interface, for a subset of optimisation flags, without a good reason too. Especially if it might still requires you to use standard env vars anyways.

@hebasto
Copy link
Owner Author

hebasto commented Dec 6, 2023

Rebased.

@hebasto
Copy link
Owner Author

hebasto commented Dec 11, 2023

Rebased.

@hebasto
Copy link
Owner Author

hebasto commented Jan 8, 2024

Marking this PR as a draft in the light of the bitcoin#29185.

@hebasto hebasto marked this pull request as draft January 8, 2024 10:51
@hebasto hebasto force-pushed the cmake-staging branch 2 times, most recently from 67ceebb to 203a3ab Compare January 16, 2024 13:23
@hebasto
Copy link
Owner Author

hebasto commented Jan 16, 2024

Closing. See bitcoin#29185.

@hebasto hebasto closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants