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 SimdArray trait and safe gather/scatter API #139

Merged
merged 6 commits into from
Jun 24, 2021
Merged

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Jun 15, 2021

This PR has four parts, without which it doesn't make a lot of sense:

  1. The introduction of the SimdArray trait for abstraction over vectors.
  2. The implementation of private vector-of-pointers types.
  3. Using these to allow constructing vectors with SimdArray::gather_{or, or_default, select}.
  4. Using these to allow writing vectors using SimdArray::scatter{,_select}.

@programmerjake
Copy link
Member

Naming question: Do we want the gather/gather_or methods to take a mask? or will the masked variant be named gather_masked?

Maybe like:

// excuse my syntax
impl ... for SimdUsize<...> {
    fn gather_or<T>(self, slice: &[T], mask: ..., fallback: Simd<[T; LANES]>) -> Simd<[T; LANES]>;
    fn gather_or_default<T>(self, slice: &[T], mask: ...) -> ...;
}

@workingjubilee workingjubilee added the I-nominated We should discuss this at the next weekly meeting label Jun 16, 2021
@workingjubilee
Copy link
Member Author

workingjubilee commented Jun 21, 2021

discussed in meeting, consensus was largely...

On the trait

  1. SimdArray is an okay name, but we may want to rename SimdArray to Vector in the very near future (like, right before debut to nightly), just to find out the Rust community's opinion on it. :^)
  2. Having methods on a trait is fine, but we probably want to max out on a very small number of "big" crate-defined expected-to-be-imported traits for users though. Ideally just 2: {Vector,SimdArray} and perhaps {Mask,SimdMask}?
  3. We'd like inherent implemented traits in a bright and shiny future but it's fine if they're not, as long as we're very disciplined about limiting required traits to use our API, see previous remark.
  4. There was some concurrence re: signal to noise ratio in an API based on the number of methods and how "partitioned" they were (or not, as the case may be) in the documentation.
  5. Maybe someday we'll have SimdInt or SimdFloat trats and the like, but definitely not in this crate for now.

On the methods

  1. No "default, unmasked" fn gather, call it fn gather_or_default instead.
  2. And it's fine if fn scatter has no exact parallel.
  3. fn gather_select and fn scatter_select will be the masked versions.

This provides a general framework for describing relationships
between vector types and scalar types.
@workingjubilee workingjubilee removed the I-nominated We should discuss this at the next weekly meeting label Jun 22, 2021
@workingjubilee workingjubilee changed the title Add SimdArray trait and safe gather API Add SimdArray trait and safe gather/scatter API Jun 22, 2021
/// A representation of a vector as an "array" with indices, implementing
/// operations applicable to any vector type based solely on "having lanes",
/// and describing relationships between vector and scalar types.
pub trait SimdArray<const LANES: usize>: crate::LanesAtMost32
Copy link
Member

Choose a reason for hiding this comment

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

Looking this over again, shouldn't the lane count be an associated const?

pub trait SimdArray {
    const LANES: usize;
}

It's impossible to implement SimdArray with different lengths for a single type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm... is that a bug?

Copy link
Member Author

@workingjubilee workingjubilee Jun 22, 2021

Choose a reason for hiding this comment

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

I am OK with using an assoc const fwiw, I just genuinely don't know which one would actually be best here. My operating assumption is that they have different implications re: const unification and type inference and I don't know what actually will offer the more ergonomic API until we rework more stuff.

Copy link
Member

Choose a reason for hiding this comment

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

The associated const would let you do something like <T as SimdArray>::LANES which I could see being useful. No easy way to extract the lane count otherwise, I think.

Copy link
Member

@thomcc thomcc Jun 22, 2021

Choose a reason for hiding this comment

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

Also it follows the best practice of "if you want to allow this type to implement the trait multiple times, use a type const parameter, otherwise use an associated type const".

EDIT: lol i said it backwards first.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add the assoc const on its own but removing the const from the trait definition induces a cycle in evaluating the well-formedness of the const generic, so I think it can't be done.

Copy link
Member

Choose a reason for hiding this comment

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

I'll check it out in a bit, just to see if I can convince it. I don't see why it shouldn't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @rust-lang/project-const-generics since we seem to be running into an amusing niche case in our expectations of how const things work and interact with Rust programming idioms, vs. how they do in practice.

for note: my attempt was to bound everything currently described by LANES on Self::LANES and instead write stuff like

impl SimdArray for Simd{Ty}<LANES> {
    const LANES: usize = LANES;
}

and I experimented with a few different configurations of where-bounds and the like, but I kept hitting cycles or outright failures to type check.

Copy link
Member Author

Choose a reason for hiding this comment

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

After finding that the problem is probably in the bounds not being moved mostly into the implementations of the traits, we will probably catch up to refactoring this in after a bit more tinkering around.

@workingjubilee workingjubilee merged commit b5ba195 into master Jun 24, 2021
workingjubilee added a commit that referenced this pull request Jul 24, 2021
Combine LanesAtMost32 and SimdArray into a single trait "Vector"

Attempts to fix some unresolved questions in #139 regarding `SimdArray` having a generic parameter.

In particular, this made it not appropriate for replacing `LanesAtMost32`.  Additionally, it made it impossible to use in a context where you otherwise don't know the lane count, e.g. `impl Vector`.

An unfortunate side effect of this change is that scatter/gather no longer work in the trait (nor does anything else that references the lane count in a type.  This requires the super-unstable `const_evaluatable_checked` feature).

I also threw in the change from `as_slice` to `as_array` as discussed in zulip, and fixes #51.
@calebzulawski calebzulawski deleted the feat/gather branch August 7, 2021 18:19
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.

6 participants