Skip to content
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 core::intrinsics::simd #118853

Merged
merged 8 commits into from
Dec 19, 2023
Merged

Conversation

calebzulawski
Copy link
Member

Intended to close rust-lang/portable-simd#381.

r? ralfjung

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 12, 2023
@RalfJung
Copy link
Member

Cc @rust-lang/opsem

@workingjubilee workingjubilee added A-ci Area: Our Github Actions CI A-simd Area: SIMD (Single Instruction Multiple Data) PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) T-opsem Relevant to the opsem team and removed A-ci Area: Our Github Actions CI labels Dec 14, 2023
library/core/src/intrinsics/simd.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics/simd.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

🤨 I forgot about that "fun" detail. Nevermind, I guess.

@workingjubilee
Copy link
Member

actually, while we're here, does anyone know if these are actually incompatible signatures, or is that lint just overly strict...?

@digama0
Copy link
Contributor

digama0 commented Dec 15, 2023

doesn't that mean the rename should also happen in portable-simd?

I can't imagine that that generic parameter names would matter here, but it still seems like a good idea to ensure the names are the same for clarity.

@RalfJung
Copy link
Member

RalfJung commented Dec 15, 2023 via email

library/core/src/intrinsics/simd.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics/simd.rs Show resolved Hide resolved
/// The bitmask is always packed into the smallest/first bits, but the order is LSB-first for
/// little endian and MSB-first for big endian.
/// In other words, the LSB corresponds to the first vector element for little endian,
/// and the last vector element for big endian.
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea what this means.^^ There's too many different notions of "first" here. Also, there's two cases to consider (output a packed int vs output an array).

What about something like this:
No matter whether the output is an array or an unsigned integer, it is treated as a single contiguous list of bits. The bitmask is always packed on the least-significant side of the output, and padded with 0s in the most-significant bits. The order of the bits depends on endianess:

  • On little endian, the least significant bit corresponds to the first vector element.
  • On big endian, the least significant bit corresponds to the last vector element.

I think this also needs examples to have any chance of being comprehensible.

As always, please first state the types and then start discussing the details; without knowing what U is the rest of this is even harder to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is much better. I took your text and added a simple example

library/core/src/intrinsics/simd.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics/simd.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics/simd.rs Outdated Show resolved Hide resolved
library/core/src/intrinsics/simd.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@calebzulawski
Copy link
Member Author

I thought those intrinsics were added? Or is this something bootstrap compiler related?

@RalfJung
Copy link
Member

Yeah this probably needs #[cfg(not(bootstrap))]

@RalfJung
Copy link
Member

Looking good. :) We can always refine this later if more things come up.
I assume the next step is to change portable-simd to import this module rather than directly importing the intrinsics?

@bors r+

@bors
Copy link
Contributor

bors commented Dec 19, 2023

📌 Commit e61aaf9 has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2023
@calebzulawski
Copy link
Member Author

Looking good. :) We can always refine this later if more things come up. I assume the next step is to change portable-simd to import this module rather than directly importing the intrinsics?

Yep! And one more feature gate to remove

@bors
Copy link
Contributor

bors commented Dec 19, 2023

⌛ Testing commit e61aaf9 with merge 558ac1c...

@bors
Copy link
Contributor

bors commented Dec 19, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 558ac1c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2023
@bors bors merged commit 558ac1c into rust-lang:master Dec 19, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (558ac1c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [0.5%, 3.9%] 5
Regressions ❌
(secondary)
3.9% [2.6%, 4.5%] 3
Improvements ✅
(primary)
-2.0% [-2.0%, -2.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [-2.0%, 3.9%] 6

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.5% [-0.5%, -0.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.757s -> 673.009s (0.04%)
Artifact size: 312.46 MiB -> 312.42 MiB (-0.01%)

///
/// `mask` must only contain `0` or `!0` values.
#[cfg(not(bootstrap))]
pub fn simd_masked_load<V, U, T>(mask: V, ptr: U, val: T) -> T;
Copy link
Member

Choose a reason for hiding this comment

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

In implementing this, I am confused. Isn't this equivalent to simd_gather?

Copy link
Contributor

Choose a reason for hiding this comment

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

gather accepts a vector of N pointers, where each element will be loaded from its corresponding pointer.

masked_load accepts a single pointer and all elements of the resulting vector (when unmasked) are loadded from a constant offset from that pointer. i.e the first element will be loaded from ptr, second from ptr.offset(1), and so on.

Copy link
Member

Choose a reason for hiding this comment

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

Then the documentation here is wrong. For masked_load it says

/// `U` must be a vector of pointers to the element type of `T`, with the same length as `T`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks like we missed this. I'll create a follow up PR soon

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-simd Area: SIMD (Single Instruction Multiple Data) merged-by-bors This PR was explicitly merged by bors. PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move intrinsic declarations to core and verify behavior
9 participants