-
Notifications
You must be signed in to change notification settings - Fork 13.4k
x86 (32/64): go back to passing SIMD vectors by-ptr #141309
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
base: master
Are you sure you want to change the base?
Conversation
@bors try |
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... 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
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.
The code changes LGTM assuming the try job passes. However, I think it's worth waiting until 1-2 more people weigh in and agree this is worth a revert.
☀️ Try build successful - checks-actions |
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.
Agree that we should go back to the old ABI for now. Can always switch back after the LLVM issue is fixed.
Is there an LLVM issue we can reference? |
We got @nikic's +1. Anyone else we should ask? FWIW this will also help with #141848: for people that disable SSE on an i686 target, the lint at #116558 (hard error on nightly and beta) can actually trigger for "Rust" ABI functions. We don't actually support such configurations, but we did not intent for them to become a hard error so quickly either. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
(That's rust-lang/miri#4367) |
We may even consider backporting this, to avoid Rust 1.88 being the only release where a "Rust" ABI function with a by-value SIMD vector on an i686 target modified to disable SSE causes a hard error. (That's not a configuration we intend to support, but it wasn't really meant to hard-error quite so quickly.) See #141848 for context. |
I think it's fine to merge with the CI fix 👍 a few other people have looked at this and upvoted the backport, so seems like there's more consensus there anyway. |
Which CI fix? This should be good to go as-is.
|
Oops I missed that the linked Miri issue has since been fixed. |
(mistyped the bors command) @bors r=tgross35,nikic |
lgtm |
…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
Rollup of 6 pull requests Successful merges: - #140715 (Clarify &mut-methods' docs on sync::OnceLock) - #141309 (x86 (32/64): go back to passing SIMD vectors by-ptr) - #141677 (Async drop - type instead of async drop fn, fixes #140484) - #141733 (C-variadic functions must be unsafe) - #141858 (Fix typo in `StructuralPartialEq` docs) - #141874 (add f16_epsilon and f128_epsilon diagnostic items) r? `@ghost` `@rustbot` modify labels: rollup
@bors rollup=iffy
|
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: x86_64-gnu-nopt
try-job: dist-i586-gnu-i586-i686-musl
try-job: x86_64-msvc-1