Skip to content

Add lto scons option#1601

Merged
dsnopek merged 1 commit into
godotengine:masterfrom
Ivorforce:lto
Nov 12, 2024
Merged

Add lto scons option#1601
dsnopek merged 1 commit into
godotengine:masterfrom
Ivorforce:lto

Conversation

@Ivorforce

@Ivorforce Ivorforce commented Sep 21, 2024

Copy link
Copy Markdown
Member

The behavior mimics almost the exact behavior (minus "progress" options) of Godot scons:

This is an active change (edit: not anymore, defaults to 'none' by default now): Once merged, all projects depending on godot-cpp (and updating the submodule) will gain LTO for release builds. I tested a similar setup in my own project, not the exact same though (because I don't have all combinations of windows, msvc, mingw and use_llvm available). However, since that it mimics what Godot does already, it should be good, given I did not make mistakes.

One important difference is that in my implementation (edit: not anymore, consistently 'none' by default now), "auto" is consistent across platforms ("full" in production, "none" in dev). I saw small, but consistent gains in all platforms, and don't see why LTO should be avoided in some platforms by default.

@Ivorforce
Ivorforce requested a review from a team as a code owner September 21, 2024 10:57
@Ivorforce

Copy link
Copy Markdown
Member Author

Not sure why the windows builds are failing. I don't have this problem with any of my git runners. If this is related to the PR, we can default to no LTO on windows. Let me know what you think.

@Faless Faless left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good overall 🚀 (haven't tested a build yet).

Some changes are needed to match the default values with upstream godot.

Comment thread tools/common_compiler_flags.py
Comment thread tools/common_compiler_flags.py Outdated
Comment thread tools/common_compiler_flags.py Outdated
Comment thread tools/common_compiler_flags.py Outdated
Comment thread tools/common_compiler_flags.py
Comment thread tools/godotcpp.py Outdated
Comment thread tools/godotcpp.py Outdated
@dsnopek

dsnopek commented Oct 5, 2024

Copy link
Copy Markdown
Collaborator

Code-wise, this seems to be getting there!

However:

  1. It'd be great if @Faless could give it one last review, and
  2. This will need to be squashed into a single commit before it can be merged - see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

@Faless Faless left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code looks correct, but the commit message seems wrong?

The message states:

Add lto scons option, defaulting to "auto" ("full" in production, "none" in dev builds).

But there is no "production" option, and the default is none.

@Ivorforce

Copy link
Copy Markdown
Member Author

Right, i forgot to change the final commit message after the changes. Should be good now!

@Ivorforce
Ivorforce requested a review from Faless November 12, 2024 12:29

@Faless Faless left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Nice work!

@dsnopek

dsnopek commented Nov 12, 2024

Copy link
Copy Markdown
Collaborator

Thanks!

@dsnopek
dsnopek merged commit c20a84e into godotengine:master Nov 12, 2024
@dsnopek dsnopek added enhancement This is an enhancement on the current functionality cherrypick:4.2 cherrypick:4.3 labels Nov 12, 2024
@dsnopek dsnopek added this to the 4.x milestone Nov 12, 2024
@Ivorforce
Ivorforce deleted the lto branch November 12, 2024 17:16
@dsnopek

dsnopek commented Jan 27, 2025

Copy link
Copy Markdown
Collaborator

Cherry-picked for 4.2 in PR #1694

@dsnopek

dsnopek commented Jan 27, 2025

Copy link
Copy Markdown
Collaborator

Cherry-picked for 4.3 in PR #1695

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement This is an enhancement on the current functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants