-
Notifications
You must be signed in to change notification settings - Fork 13.4k
new warnings about #[inline(always)]
functions missing #[target_feature(enable="sse")]
#141848
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
Comments
This comment has been minimized.
This comment has been minimized.
Uh, wait, something is very strange here. You are not using the "C" ABI it seems? It shouldn't be possible to hit this warning with the "Rust" ABI... Oh. You are using |
So the actual problem is this:
This warning is emitted so early it's not affected by That also explains why you only started seeing this recently. The "good" news is that #141309 will make the errors go away. But the warning I just quoted will stay, and we do plan to make it a hard error eventually. Building code without SSE2 should use an i586 target, which are generally lower-tier reflecting the fact that our support for such CPUs is not great (not least because LLVM has a long-standing soundness issue for such targets caused by use of the x87 FPU -- there's apparently not enough people interested in such retro chips to keep them well-supported). |
Thanks for looking at this so closely. As you can probably tell, I'm a bit out of my depth here :) I'm only just now understanding that this top-level warning (which you said will become an error?) has nothing really to do with my code, and actually repros for an empty project:
This happens regardless of whether the For context, I added this particular test target long ago, and I can't even remember how I picked it. We might have other options for working around this issue, including:
Just switching to |
This started emitting warnings recently, and in the future those will be hard errors. See rust-lang/rust#141848. The i586 target triple still works, and it's sufficient for exercising the non-SSE2 case. Fixes #489.
I have to answer this with a counter-question -- what are you trying to achieve by setting that target CPU? If you truly want to support CPUs that don't have SSE2, then yes the recommendation is to switch to an If you just want to test those codepaths, you could have a cargo feature flag or |
Fair question. My instinct for now is that we want to keep actually building for this ancient target, so that at least we'll find out if/when something else breaks? I've landed BLAKE3-team/BLAKE3@acf7a54, which fixes our CI. |
Yeah that makes sense. If that becomes difficult at some point (e.g. if there's no tier 2 target left that supports non-SSE-compilation) you may just want to assert the presence of SSE2 and declare older chips unsupported. If you are distributing binaries those absolutely should be built with SSE2, or else you are risking correctness issues (that would affect all systems, not just the old ones) -- that's what led to people pushing dropping support in Debian. |
…nikic x86 (32/64): go back to passing SIMD vectors by-ptr Fixes rust-lang#139029 by partially reverting rust-lang#135408 and going back to passing SIMD vectors by-ptr on x86. Sadly, by-val confuses the LLVM inliner so much that it's not worth it... Also fixes rust-lang#141848 by no longer actually using vector registers with the "Rust" ABI. r? `@tgross35` Cc `@nikic` try-job: `test-various*` try-job: x86_64-gnu-nopt try-job: dist-i586-gnu-i586-i686-musl try-job: x86_64-msvc-1
x86 (32/64): go back to passing SIMD vectors by-ptr Fixes #139029 by partially reverting #135408 and going back to passing SIMD vectors by-ptr on x86. Sadly, by-val confuses the LLVM inliner so much that it's not worth it... Also fixes #141848 by no longer actually using vector registers with the "Rust" ABI. r? `@tgross35` Cc `@nikic` try-job: `test-various*` try-job: dist-i586-gnu-i586-i686-musl try-job: `x86_64-*` try-job: `i686-*`
x86 (32/64): go back to passing SIMD vectors by-ptr Fixes #139029 by partially reverting #135408 and going back to passing SIMD vectors by-ptr on x86. Sadly, by-val confuses the LLVM inliner so much that it's not worth it... Also fixes #141848 by no longer actually using vector registers with the "Rust" ABI. r? `@tgross35` Cc `@nikic` try-job: `test-various*` try-job: dist-i586-gnu-i586-i686-musl try-job: `x86_64-*` try-job: `i686-msvc*`
x86 (32/64): go back to passing SIMD vectors by-ptr Fixes #139029 by partially reverting #135408 and going back to passing SIMD vectors by-ptr on x86. Sadly, by-val confuses the LLVM inliner so much that it's not worth it... Also fixes #141848 by no longer actually using vector registers with the "Rust" ABI. r? `@tgross35` Cc `@nikic` try-job: `test-various*` try-job: dist-i586-gnu-i586-i686-musl try-job: x86_64-gnu-nopt try-job: `x86_64-msvc*` try-job: `i686-msvc*`
…ngjubilee x86 (32/64): go back to passing SIMD vectors by-ptr Fixes rust-lang/rust#139029 by partially reverting rust-lang/rust#135408 and going back to passing SIMD vectors by-ptr on x86. Sadly, by-val confuses the LLVM inliner so much that it's not worth it... Also fixes rust-lang/rust#141848 by no longer actually using vector registers with the "Rust" ABI. r? `@tgross35` Cc `@nikic` try-job: `test-various*` try-job: dist-i586-gnu-i586-i686-musl try-job: x86_64-gnu-nopt try-job: `x86_64-msvc*` try-job: `i686-msvc*`
Apologies if this code was always broken and I just don't know it yet :) Here's a CI run that recently started failing, due to new warnings plus our usual
RUSTFLAGS: "-D warnings"
: https://github.com/BLAKE3-team/BLAKE3/actions/runs/15326026728/job/43120654065. Reproducing that locally:The code in that file is divided into two types of functions, public ones annotated with
#[target_feature(enable = "sse2")]
, and private ones annotated with#[inline(always)]
. I had thought (or read somewhere) thatinline(always)
functions assumed the CPU features of their caller and didn't need to be annotated. Either way it seems to be incompatible withtarget_feature
. What's should this code be doing differently?The text was updated successfully, but these errors were encountered: