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

Tweak names of wasm SIMD intrinsics #1096

Merged
merged 4 commits into from
Mar 25, 2021

Conversation

alexcrichton
Copy link
Member

The original intention of wasm SIMD support was to very closely match
the spec 1:1 to provide low-level primitives. Over time though this has
become less appealing and "niceties" such as i64x2_shuffle have been
added instead of only giving users i8x16_shuffle (same thing for
*_const functions).

This commit goes through and gives special treatment to all SIMD
intrinsics and instructions for wasm. This performs changes such as:

  • All *_u and *_s suffixes are now baked into the intrinsic type,
    e.g. changing i16x8 to u16x8 where appropriate.
  • *_const functions have been added for unsigned types
  • *_replace_lane functions have been added for unsigned types
  • *_extract_lane functions have been added for unsigned types

While I think it makes sense at the wasm simd spec layer that all these
instructions have the names they do, I've concluded that at the Rust
layer we'll want more understandable names since they're so easy to bind
and are easier to understand when reading/writing code.

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@CryZe
Copy link
Contributor

CryZe commented Mar 24, 2021

Are these only being renamed in Rust or C / C++ as well? Cause I find it a bit odd for Rust to diverge from C's intrinsics when this was something that was explicitly decided against for x86.

The original intention of wasm SIMD support was to very closely match
the spec 1:1 to provide low-level primitives. Over time though this has
become less appealing and "niceties" such as `i64x2_shuffle` have been
added instead of only giving users `i8x16_shuffle` (same thing for
`*_const` functions).

This commit goes through and gives special treatment to all SIMD
intrinsics and instructions for wasm. This performs changes such as:

* All `*_u` and `*_s` suffixes are now baked into the intrinsic type,
  e.g. changing `i16x8` to `u16x8` where appropriate.
* `*_const` functions have been added for unsigned types
* `*_replace_lane` functions have been added for unsigned types
* `*_extract_lane` functions have been added for unsigned types

While I think it makes sense at the wasm simd spec layer that all these
instructions have the names they do, I've concluded that at the Rust
layer we'll want more understandable names since they're so easy to bind
and are easier to understand when reading/writing code.
@alexcrichton
Copy link
Member Author

Personally I would hope that each architecture inside of the core::arch module can determine its own conventions as appropriate. For example x86 has thousands of intrinsics and the libs team didn't want to review them all individually so it made sense to use the preexisting standard of the intrinsic header files. In isolation, though, the x86 names don't make a ton of sense in Rust (e.g. they don't need mm prefixes and all that) and ideally we would have thought of better names for them (given infinite time)

For wasm we don't have the convenience of a preexisting standard to copy from, and additionally we're working with the scale of hundreds of intrinsics instead of thousands. These two combined lead me to believe that wasm should reserve the right to blaze it's own trail here and set the precedent. Functions like memory_grow and memory_size are trying to match the name of the instruction they're matching, but such a convention is not necessarily the best here given the simd intrinsics.

All that to say that while I think we can take inspiration from what clang is doing for C I do not think we should be bound to follow it exactly. For example if you look at clang's header file for wasm_simd128.h you'll notice that all functions are prefixed with wasm_* which I don't think makes sense in Rust. Furthermore the header defines a number of vector types for each lane/type pair, but I don't think that makes sense in Rust because the implicit conversions in C aren't available in Rust (and would make the Rust API more verbose).

To reiterate, though, there's no point in intentionally being different from clang. It's not like clang made bad decisions or anything like that, my point is just that the decisions for C-the-language are not always the right decisions for Rust-the-language as well. As such I'm of the opinion that we should do what's right for Rust in this module, not just copy C. Language differences largely come up when handling types/conversions in this instance, but if you look at the header file for C you'll find that this PR actually brings most intrinsics to match names with the C header file.

@Amanieu
Copy link
Member

Amanieu commented Mar 24, 2021

Looking at the Clang header, it uses vNxM instead of iNxM when the signedness doesn't matter for the operation. Perhaps we should do the same? It feels a bit strange that some iNxM function have a uNxM counterpart while others don't.

@alexcrichton
Copy link
Member Author

I was just wondering that myself this morning about the v* prefix. It's worth noting that the wasm_simd128.h header has not been updated for the latest batch of renamings in the wasm simd proposal. Some functions there mirror old instruction names which have since been renamed. For example wasm_v8x16_shuffle mirrors the old v8x16.shuffle instruction, but it was decided to rename the wasm instruction in WebAssembly/simd#316. From reading that issue it appears that the main motivation is that by renaming shuffles and loads it was possible to remove the vNNxMM syntax entirely from the wasm spec.

I think that this naming only really applies to *_load_splat and *_shuffle, right? I'm personally fine keeping the v* prefix there since I do think it helps indicate that signedness doesn't matter, it's just about lanes of that width.

@Amanieu
Copy link
Member

Amanieu commented Mar 24, 2021

The vNxM naming could also apply to vNxM_eq/vNxM_ne, but that might be confusing since we do care about the type a bit. I'm not feeling too strongly about vNxM so let's just keep it as iNxM to match the spec.

@alexcrichton
Copy link
Member Author

For comparisons we it would also apply only to integers because f64x2_eq naturally has different semantics than i64x2_eq.

I do find this an oddity though where if you use u32x4_const to create a vector you'd then execute i32x4_add to add two together. That originally made me hesitant to make this change but in the end I figured that at least for the sign-suffixed cases it just seems to universally make more sense to factor the sign into the type at a programming-language level for readability. We could always add u32x4_add in the future if necessary.

@CryZe
Copy link
Contributor

CryZe commented Mar 24, 2021

In general I'm definitely in favor of cleaning up the API, in fact I was about to write a clean wrapper API myself (probably as part of safe_arch which does this already for x86). So if this is happening in std, that would be even better. Though it's sad that this did not happen for x86.

Though I would then also be in favor of adding proper f32x4, f64x2, ... types. Most of the transmuting will come from iterating e.g. [f32] slices as f32x4 chunks. For this you will need to have some transmute solution anyway. I'd argue that e.g. using f64x2 intrinsics on f32x4 vectors is so rare that you also probably should be transmuting, just so the compiler can catch the likely mistake that this is otherwise. Also it just straight up seems weird to willingly introduce a weakly typed API into Rust which otherwise is very strongly typed (neither x86 nor ARM SIMD is this weakly typed either). It probably makes more sense to just introduce functions / methods on those types for safely converting between them as part of the std::arch::wasm32 API if the transmutation is considered too much of a problem.

In fact someone recently wrote a more strongly typed WASM SIMD header for C for exactly this reason: https://github.com/nemequ/wav

@alexcrichton
Copy link
Member Author

This is also one of the major reasons x86 didn't do anything more than the header file, it allows libstd to entirely side-step the question of "what if it were actually nice to use simd?" and instead just provides the tools to build something nice externally (hoping that someone would do so). Now that being said I think that building something nice externally for x86 is a lot more difficult than it is for wasm (at least as wasm simd stands today).

I'm personally not entirely sure what the best direction to go in wasm is. The wav project you pointed out I think is the exact API we'd want (more-or-less) in Rust if the goal of std was to have an ergonomic API for exposing all these intrinsics. A wav-like API built on what std currently has, however, could certainly also exist on crates.io.

The cost of doing something wav-like in this crate (and libstd) is basically just effort for stabilization. It would require much more scrutiny. Additionally I don't think it's obvious/clear that such an abstraction would be able to live on into the future. For example I couldn't say with certainty that all future wasm simd functionality could live in a well-typed API like that.

I think that the current state of the core::arch::wasm32 module is a bit of a weird middle-ground. It doesn't exactly match the "specification" (which in this case would be the online specification, not C, so all we have is instructions and type signatures), but it also doesn't go the full 9 yards of being a nice and safe API to use. Instead it only provides some mild conveniences in a few locations. This feels weird to me because it's trying to be more ergonomic but it's unclear where the line is drawn and why at this point in time.

I don't really know what to do about any of this myself. I want wasm simd on a track to stable in a reasonable time frame, especially given that engines are picking up steam implementing the proposal. Beyond that though I don't know what the best long-term option is for this module.

@alexcrichton
Copy link
Member Author

Ok, should be green now!

I initially tried to get the *_unaligned changes here too but that led to odd link errors which I don't feel like debugging right now.

@Amanieu Amanieu merged commit b5b3787 into rust-lang:master Mar 25, 2021
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.

4 participants