-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add feature for basis u compilation with SSE 4.1 on supported platforms #14
base: master
Are you sure you want to change the base?
Conversation
Steam hardware survey has support at >99% so I think this is fine. If the CI fails again I'll grab and test this on mac myself. Thanks! |
(I think bevy is using this repo so I asked in their discord for anyone with objections to speak up.) |
It seems preferable to check whether |
basis-universal-sys/build.rs
Outdated
@@ -15,10 +15,12 @@ fn build_with_common_settings() -> cc::Build { | |||
} | |||
|
|||
fn main() { | |||
let sse_support = cfg!(target_feature = "sse4.1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that cfg!
will detect the features used to build the build script itself, which are not necessarily the same as the target building for (for example when --target
is used Cargo will not pass the rustflags to the build script, but it will pass them to the crate you're building for). It's suggested instead to read the CARGO_CFG_TARGET_FEATURE
environment variable, which will properly report the target features enabled for the target you're building for.
Something like:
let sse4.1 = std::env::var("CARGO_CFG_TARGET_FEATURE")
.expect("Cargo didn't set the CARGO_CFG_TARGET_FEATURE env var")
.split(',')
.any(|feature| feature == "sse4.1");
Someone please correct me if I get this wrong but the tier 1 targets in rust are x86_64, i686, and aarch64. You can check features with something like
None of these tier 1 targets include sse4.1. So while checking the target does seem like the intended solution, in practice I think this will mean almost no one will enable sse4.1. (I expect most people take the default target for their OS.) This is not to say I object to going with this, but I think it's worth mentioning. |
The default target-cpu for each of the tier 1 targets doesn't include sse4.1. However, you can set a different target-cpu via RUSTFLAGS. You can see the list of supported CPUs to pick from with something like:
The list includes a couple of "x86-64 microarchitecture levels" in addition to named cpu models. Actually using it would look something like:
(You can instead create a |
Based on the input so far, going to hold off on taking this PR until it is opt-in, preferably by checking CARGO_CFG_TARGET_FEATURE |
This reverts commit 29863c0.
Renamed feature "sse4_1" to "hint_sse4_1"
Thanks for all your input! I think letting the user pass compiler flags is too arcane, so I exposed it as a regular feature |
Should improve encoder times 15-30% acording to the original repo on supported platforms (virtually any x86 CPU made after 2007).
Whether basisu is built with SSE4.1 support is simply dictated by the build target platform, so no need to create an extra crate feature for this in my opinion. 🙂