Skip to content

Implement arithmetic operation traits for x86 SIMD types#1898

Closed
sayantn wants to merge 1 commit into
rust-lang:masterfrom
sayantn:arith-ops
Closed

Implement arithmetic operation traits for x86 SIMD types#1898
sayantn wants to merge 1 commit into
rust-lang:masterfrom
sayantn:arith-ops

Conversation

@sayantn

@sayantn sayantn commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

ACP: rust-lang/libs-team#628
Tracking Issue: rust-lang/rust#145066

Unresolved questions: Should we have Not and Rem implementations? At least Not seems quite useful.

I will send the PRs of other archs after this is merged, as they also use the common macro

@rustbot

rustbot commented Aug 7, 2025

Copy link
Copy Markdown
Collaborator

r? @folkertdev

rustbot has assigned @folkertdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@sayantn

sayantn commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

CI is stuck on #1897 🤦🏽

@folkertdev

Copy link
Copy Markdown
Contributor

yeah that will likely take a couple of days at least

macro_rules! impl_arith_op {
(__internal $op:ident, $intrinsic:ident $_:ident) => {
#[inline]
fn $op(self, rhs: Self) -> Self {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are missing a #[target_feature(enable = "...")] matching the _mm* caller, so the MIR inliner will not inline them and LLVM may not inline them either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

simd_* intrinsics call the platform-independent LLVM intrinsics/instructions. The conversion to ASM checks the available feature flags, and uses them as available. See rust-lang/libs-team#628 (comment)

@folkertdev folkertdev closed this Aug 20, 2025
@folkertdev folkertdev reopened this Aug 20, 2025
@folkertdev

Copy link
Copy Markdown
Contributor

Do you want to decide on those unresolved questions here, or later, or bounce it back to T-libs?

  • simd_rem is not really used (I find 2 occurrences), suggesting that there is no instruction support for this operation and we should probably steer users away from it
  • we don't have a simd_not, but simd_xor(v, splat(all_one_bits)) should do something efficient

@folkertdev

Copy link
Copy Markdown
Contributor

Also @Amanieu merging this PR insta-stabilizes these instances, so just checking: that is intentional, right?

I feel like in that case we should at least implement the same operations for all stable simd types in the same rust release, i.e. at least make sure the aarch64 ones make it into the same release as the x86 ones.

@Amanieu

Amanieu commented Aug 24, 2025

Copy link
Copy Markdown
Member

Any instantly-stable API needs a libs-api FCP, even if an ACP was accepted.

I can nominate this for libs-api discussion around Not/Rem, but I believe at least in the last discussion we decided against having Rem since there's no hardware support for it on any architecture.

Personally, I'm having second thoughts about having these trait implementations at all: I feel that in the long term what you really want to do is use the portable SIMD API most of the time and only occasionally dip into the target-specific intrinsics where needed. I also feel that these trait implementations can be a performance footgun because they may give you the impression that SIMD support is available for an operation (e.g. division) when it isn't actually supported by hardware.

@sayantn

sayantn commented Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

Closing this because the ACP was rejected after further thought

@sayantn sayantn closed this Sep 11, 2025
@sayantn sayantn deleted the arith-ops branch October 8, 2025 10:32
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