Skip to content

use simd_saturating_{add, sub} on neon #1575

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

The idea behind this is that e.g. miri and cranelift don't know all of the llvm intrinsics (and adding them is possible but a lot of extra work). Using the generic simd function means logic only needs to be implemented once for all targets.

The simd_saturating_* functions generate the same instructions for the saturating add/sub operations https://godbolt.org/z/zc35doTc6

@rustbot
Copy link
Collaborator

rustbot commented May 30, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

@folkertdev
Copy link
Contributor Author

I don't think this idea works because of this LLVM issue I opened llvm/llvm-project#94463. So unless we can get that fixed (or somehow accept much worse codegen) this PR does not work

@Amanieu
Copy link
Member

Amanieu commented Jun 7, 2024

OK let's defer this for now until the LLVM issue is sorted.

@folkertdev
Copy link
Contributor Author

@Jamesbarford given that you work at ARM, is there any chance you could pass this on to the right person (specifically the LLVM issue llvm/llvm-project#94463). Not only does it clean up stdarch a bit, but it's especially valuable for std::simd.

@Jamesbarford
Copy link
Contributor

Sure, I've mentioned it 👍

auto-merge was automatically disabled June 1, 2025 22:02

Head branch was pushed to by a user without write access

@folkertdev folkertdev force-pushed the neon-simd-saturating-add-sub branch from 86450b2 to fe53529 Compare June 1, 2025 22:02
@sayantn
Copy link
Contributor

sayantn commented Jun 2, 2025

We can still use simd_saturating_add for arm, right? There are actually quite a few such occurrences in ARM (e.g. using llvm.fma instead of just using simd_fma). Also there are a few which use wrong LLVM intrinsics, I will send a PR to correct those soon

@folkertdev
Copy link
Contributor Author

I actually fixed llvm/llvm-project#94463 in llvm/llvm-project#140454. I just re-ran CI here to check if there are any other issues, and there were, resulting in llvm/llvm-project#142323 and a fix in llvm/llvm-project#142342.

So, things are looking good for being able to use these operations for aarch64 with the next llvm release.

For arm: we can use the platform-independent versions if they don't cause regressions. If that's the case (looks like it for these operations at least) then yes, we can (and should!) use the simd_ intrinsics.

@sayantn
Copy link
Contributor

sayantn commented Jun 2, 2025

The ARM intrinsics are actually using the same LLVM intrinsics that simd_saturating_{add,sub} use, so it shouldn't cause any regressions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants