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

SCons: Properly set SSE2 as baseline on x86_32 #100337

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Dec 12, 2024

Setting it only for release templates on Windows and macOS was inconsistent, and Jolt requires it as a minimum.

Drop the -mxsave flag from the raycast module, this doesn't seem to be used explicitly by Embree, and unnecessarily makes our config and baseline muddy.

I'm aware of #59595, but this one requires more discussion as requiring SSE4.2 does cut off a significant amount of CPUs, and we need to evaluate whether we're willing to make that compromise.

SSE2 on the other hand should be a no brainer, it was already the default for MSVC, and for all other platforms indirectly by way of it being set for the raycast module, which would mean that our binaries can't run on i386 machines without SSE2 already.

@akien-mga akien-mga added bug topic:buildsystem topic:porting cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Dec 12, 2024
@akien-mga akien-mga added this to the 4.4 milestone Dec 12, 2024
@akien-mga akien-mga requested a review from a team as a code owner December 12, 2024 21:26
Copy link
Member

@hpvb hpvb left a comment

Choose a reason for hiding this comment

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

At the risk of bikeshedding, I think we might as well just set -mssse3 that is supported by every CPU that has at least 2 cores. The newest cpu to support it is AMD's "bobcat" architecture which is from early 2011.

Every single "atom" branded CPU has it too. On the Intel side this makes the oldest required CPU a "Core 2 duo" or a 64bit capable P4.

Now I don't want this to bikeshed forever, I am just approving this. But I would also just approve an -mssse3 if presented!

@akien-mga akien-mga changed the title SCons: Properly set SSE2 as baseline on x86_32 and x86_64 SCons: Properly set SSE2 as baseline on x86_32 Dec 12, 2024
@akien-mga
Copy link
Member Author

At the risk of bikeshedding, I think we might as well just set -mssse3 that is supported by every CPU that has at least 2 cores. The newest cpu to support it is AMD's "bobcat" architecture which is from early 2011.

Every single "atom" branded CPU has it too. On the Intel side this makes the oldest required CPU a "Core 2 duo" or a 64bit capable P4.

I would agree too. I propose we do this in two steps so we get an intermediate dev snapshot inbetween for testing purposes, if we need to debug reports of some users having Godot suddenly crash on their old PC.

This PR just affects x86_32 in a way that should practically be a no-op.

Then we can make another PR bumping to ssse3 that will affect both x86_32 and x86_64.
Or would we bump to ssse3 only for x86_64?

@hpvb
Copy link
Member

hpvb commented Dec 12, 2024

Then we can make another PR bumping to ssse3 that will affect both x86_32 and x86_64. Or would we bump to ssse3 only for x86_64?

I'd do both x86_32 and x86_64. There's some reason to want to run a 32bit linux distro in particular on a machine that old. Moving from SSE2 to SSSE3 in practice doesn't really limit the amount of CPUs that can run Godot much either way.

Setting it only for release templates on Windows and macOS was inconsistent,
and Jolt requires it as a minimum.

Drop the `-mxsave` flag from the raycast module, this doesn't seem to be
used explicitly by Embree, and unnecessarily makes our config and baseline
muddy.

if env["target"] == "template_release":
if env["arch"] != "arm64":
env.Prepend(CCFLAGS=["-msse2"])
Copy link
Member

Choose a reason for hiding this comment

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

For the reference - current min. supported macOS version (10.13) require SSE4.1 to work.

@Repiteo Repiteo merged commit ed8b4ab into godotengine:master Dec 12, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 12, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:buildsystem topic:porting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants