Repository navigation
Minimal path to stabilization #159
Description
I wanted to open a sort of tracking issue for getting stdsimd into the standard library and on the path to stable Rust. That's sort of what we've wanted all along! What I propose here may seem a bit radical, but hear me out!
First and foremost I see the purpose of the stdsimd crate to be providing definitions of the vendor intrinsics. These functions are all unsafe as previously discussed and use #[target_feature] to implement themselves. Vendors (like Intel) define how many functions are here and what their names are. The important thing about these intrinsics is that how they're defined is compiler dependent. This, in turn, forces them to be implemented with unstable functionality, namely we must move this to the standard library in one form or another to be stable.
Next up if we have vendor intrinsics we also have the types! Each vendor intrinsic has types associated with it, although we are thinking of differing from the upstream vendors with more "principled" types like u8x16 instead of __m128i. As a result these types are also defined in the standard library.
After this, however, I'd propose we stop. This issue is all about the minimal path to stabilization (while still being useful) and I believe that type definitions plus intrinsics is sufficient. I say this namely because that's all C/C++ have, type definitions and intrinsics. I personally very much want to stabilize more convenient names like f32x4::splat or u16x8::extract where I don't have to remember janky names from Intel, but this is always backwards compatible to add at a later date. I think we have a lot more to gain from a little bit of SIMD now rather than "super portable SIMD right out the gate" much later. I hope to soon after the first pass of stabilization start looking to stabilizing the more convenient and powerful APIs (like Add for u32x8).
Once we have this then I think we also need to stabilize the #[target_feature] attribute and its associated features. This namely involves finishing implementing RFC 2045, a slight tweak to #[target_feature] syntax.
Up to this point I believe the last remaining issue is "what happens on the classical ABI mismatch issue". Namely, what does rustc do with this function:
#[target_feature = "+avx"]
fn foo(a: u64x8) -> u64x8 {
a
}
unsafe fn bar() {
foo(mem::zeroed());
}(or something like that)
Notably here the bar function does not have the avx feature enabled, so the ABI it will invoke foo with differs than that of the ABI that foo is expecting, hence causing a mismatch. I would propose a different strategy from RFC 2045 talked about at Rust Belt Rust with some others, namely inserting shims. This I believe solves the function pointer problem as well! Namely:
- If you invoke a function with a different set of target features and you have an argument that's ABI-relevant, the compiler inserts a shim which "fixes" the invocation. Namely it would fix the above to:
#[target_feature = "+avx"]
fn foo(a: u64x8) -> u64x8 {
a
}
#[target_feature = "+avx"]
fn foo_shim(a: &u64x8, out: &mut u64x8) {
*out = foo(*a);
}
unsafe fn bar() {
foo_shim(mem::zeroed());
}- All function pointers have the "default" ABI in that they have no target features enabled. This means that if you coerced
footo a function pointer above you would actually get afoo_shimfunction pointer, regardless of where the function pointer is created.
When this is all assembled I believe we've got a very short path to stable SIMD on Rust today. This path should be sound so long as you have the right CPU feature checks in place, extensible to add more portable extension in the future, and complete in that we have accomodated for the existence of all vendor intrinsics.
So in summary, I believe we have three blockers for stabilization:
- Agree on the minimal API consisting purely of unsafe vendor intrinsics and type definitions, no trait impls or other methods on the type definitions stabilized yet (they can and will be added later!)
- Implement the minor tweaks to the
#[target_feature]attribute as specified in RFC 2045 - Implement generation of "shim" functions in trans to work around the "ABI mismatch" problem
Once this is all done and agreed on I think we can also look to moving this crate into the standard library. I believe that would entail basically adding a src/stdsimd submodule pointing to this repository, adding a pub mod simd to src/libcore/lib.rs which points to this crate's root (probably setting some sort of #[cfg] along the way so this crate knows what's happening), and then finally adding a bunch of stability annotations all throughout the crate.
Ok so that may be a bit much, but I'm curious what others think!
cc @gnzlbg
cc @BurntSushi
cc @nikomatsakis
cc @withouboats
cc @hdevalence
Activity
aturon commentedon Oct 28, 2017
cc @withoutboats (typo in issue)
gnzlbg commentedon Oct 28, 2017
I agree 100% with everything you said.
I'd like to add one thing:
I think we should prototype a "super portable SIMD library" that uses
stdsimdfor the intrinsics before we stabilize anything. Nothing fancy, just some vector arithmetic,sqrt, and not much more, just to check whether it can be built on top ofstdsimdor not, and if not, what the problems are.I've started already to do this and will commit it as an
example/(or if it gets too big, as its own crate as soon as we do the next release ofstdsimd), but I think doing this in parallel will either give us confidence about the design or warning signs about things we might have to tune.Just to be clear: I think a portable SIMD library does not belong in
std(there are many ways to do that, with different trade-offs), but I think we should make sure that whatever we stabilize is at least on the right path to allow building those libraries on stable Rust some day.And I have one nitpick:
I think that inserting shims is a great idea! The shims are a zero-cost abstraction, that is, if you want to do this there is no more efficient way of doing it. However, if I ever want to avoid the cost of the shims, I'd like to have enough control to do that.
So I would prefer if the compiler would hard error when this happens (so that I know where these cases are), and we would provide some functionality to manually tell the compiler "insert shims here". We can always make it automatic in the future without breaking backward compatibility.
Anyhow, I don't want to derail this thread with the nitpick, we can discuss that somewhere else.
withoutboats commentedon Oct 28, 2017
@alexcrichton This plan sounds solid to me. 👍
In terms of ordering work, does anything block putting the intrinsics into libcore behind a feature flag right now? While all of these items need to be done before stabilizing anything, I think they could be done in any order.
@gnzlbg
Since we're talking about just exposing the lowest-level platform intrinsics, I don't see a lot of choices between what paths we're on. This proposal seems like stabilizing just the things there are no alternatives between.
Do you have concrete examples of things you think are uncertain now and that you don't want to lock down through stabilization?
Isn't this supported by using the
cfgbased system (which I think is fully implemented and stable today) instead of the runtime system? That is, you cfg out the functions unless the target supports these features without any shimming.gnzlbg commentedon Oct 28, 2017
@withoutboats check out the
examples/nbody.rswhich currently uses static feature detection. We would like to improve that to select the best instruction at run-time. We can already do that today, but it is a bit unclear whether how that looks is what we want.For example, a safe SIMD library can do the runtime detection on each intrinsic call, but that's basically adding an atomic load to each simd operation in the library. Ideally, the dispatch would happen at a higher level (maybe even at the level of
fn main()). It is unclear how to write a portable SIMD library that would support doing this at this point.By cost, I meant the cost of converting e.g. 2x 128bit-wide registers into a 256 bit one and vice-versa. The shims do this conversion, which is not free. This conversion can be done manually today, e.g., by manually writing the shims. The question is whether we want to induce this cost "silently" or whether we want instead offer a way to automatically generate these shims, but such that the user has to say that it wants them. I'd rather start conservatively, but either way we are moving in the right direction. Once the shim generation is implemented we can revise whether we want to generate them automatically, or error and let the user decide (ideally in such a way that a library that does it automatically can be built on top of it).
FWIW, this means that we can stabilize the intrinsics, the vector types, and
#[target_feature]but that maybe we should probably explore writing high-level SIMD wrappers a bit before stabilizingcfg(target_feature)andcfg_feature_enabled.alexcrichton commentedon Oct 29, 2017
@withoutboats correct yeah, I think we could make inroads immediately on including this library into libcore.
@gnzlbg
I'd agree that we should play around with this interface some more! That's sort of what we expect to naturally happen as this becomes more broadly available on nightly (e.g. included into libcore). I'm a little skeptical like @withoutboats, however, that this will turn up a whole lot and I wouldn't want to necessarily block on a proof-of-concept library (depending on the circumstances)
In my mind the shims are basically necessary for this to be a "reasonable" feature in terms of usability and expectations of how Rust code works "in the large". I'd imagine that the impact of such shims would show up very rarely in practice.
gnzlbg commentedon Oct 29, 2017
Yes, I don't think we should block moving this into
coreon a prototype of a higher level library. From core to Rust stable there is more than enough time to do that.Yes, I also expect that these shims will be inserted very rarely in practice, and when it does, what most people will want is the shims to be generated anyways.
BurntSushi commentedon Oct 30, 2017
@alexcrichton Thanks for this proposal! Broadly speaking, I'm OK with this. I would however like to note some costs so that we're all on the same page.
In particular, if we omit everything except the type definitions themselves, then I think the following is true:
std, without LLVM's help) will be hard. @stoklund talks about this here: https://internals.rust-lang.org/t/getting-explicit-simd-on-stable-rust/4380/209 --- That in turn means that the currentsimdlibrary won't have an obvious path to stabilization. (This last point isn't terribly concerning, since it's not clear what role thesimdcrate will play anyway. But still, if someone wanted to go out and write their ownAddimpls, for example, then it's not trivial.)asmethods orFromimpls that we have today, I think folks will be forced to transmute or pointer cast. I think @sfackler had some thoughts on this.I do think this is a fine first step though!
sfackler commentedon Oct 30, 2017
Yeah - I was porting StreamVByte from 32 to 64 bit values. The encode kernel interprets its vectors as all of the u8x32, u32x8, i32x8, and u64x4 types. I have to convert at least one of the vector arguments to every single intrinsic call.
I don't know how common that stye of SIMD workload is, but the type safety was a net negative. If the
Intoimpls went away it'd be even worse.alexcrichton commentedon Oct 30, 2017
@BurntSushi excellent points! I'm just trying to thread the needle as "quickly as possible" in the sense of building up a conservative base that can be experimented with while ensuring we can continue to add features. I'd hope enabling the
simdcrate as-is today would be the next stop of stabilization for SIMD in Rust!@sfackler an interesting case study! Did the original code rely on implicit conversions done by the C compiler? I think we're slighly more principled than
__m256ifor example (having a bunch of widths other than that and typing the intrinsics more strictly than the Intel documentation says). In that case we're definitely going to be more painful than with C, but if the C intrinsics have different types themselves then it's sort of like porting line-for-line arithmetic in C to Rust, it just likely needs more casts.gnzlbg commentedon Oct 30, 2017
sfackler commentedon Oct 30, 2017
@alexcrichton From what I can tell, the C code is able to work entirely with
_m128is, with a union to type pun bytes and ints out: https://github.com/lemire/streamvbyte/blob/master/src/streamvbyte.c#L183.hdevalence commentedon Oct 30, 2017
My understanding is that many of the
Fromcasts currently in thestdsimdcrate are already effectively transmutes/bitcasts -- for instance,i32x8vsi64x4. Is this correct? Does this mean that the question isn't whether people will transmute or not, but just whether they'll do it usingstdsimd-provided impls?gnzlbg commentedon Oct 30, 2017
alexcrichton commentedon Oct 31, 2017
In general I just wanted to make a proposal which had stabilization as an option in the near future. We can of course add more requirements to that, for example auditing the current API of
u32x8, casts and trait impls and all. I fear that will delay stabilization further, however, and at this point I feel we have more to benefit from getting something out there rather than being perma-unstable.hdevalence commentedon Oct 31, 2017
@gnzlbg I'm not sure I understand, what do you mean by more principled?
52 remaining items
hdevalence commentedon Jan 23, 2018
The discussion I was looking for is in #251 (linked from #263), and after reading it I think that the decision to match the upstream definitions exactly is the right one.
I don't think that it's a good idea to block stabilizing vendor intrinsics until the development of a "portable" SIMD library happens. In fact I don't think they're connected at all and that it's a mistake to try to couple them together.
hsivonen commentedon Jan 24, 2018
It helps me to understand the decision, yes. Thank you.
I'm still unhappy about it, though, because even if lane-aware wrappers are developed eventually, having the non-lane-aware versions around makes it more likely that there's going to be vendor-specific code even for stuff that could be written portably using lane-aware types and the features the
simdcrate exposes.That is, I believe lane-aware types nudge towards portability for the parts that can be portable, so you end up with ISA-specific divergence for small building blocks instead of whole algorithms having totally separate code for different ISAs.
I might agree if the
simdcrate didn't exist, but when it exists and it's working so well for me, it's so frustrating that it doesn't get rubber-stamped for stable (with the vendor-specific bits that overlap with the systematic vendor intrinsic support effort removed or hidden).Having non-portable SIMD on stable and portable SIMD on nightly puts pressure on crates to go with non-portable code in order to have SIMD on stable. It's not good for the ecosystem to steer crates to be Intel-specific when they could be supporting other ISAs, most notable the ARM ones.
The portable stuff has real uses. Today, in the Firefox context on x86_64, I measured a 97.54% execution time reduction for our "Does this UTF-16 string contain right-to-left characters?" function with Thai input from replacing C++ that didn't use explicit SIMD with portable
simdcrate-using Rust code. The difference between explicit SIMD and code written as ALU code is less impressive on aarch64, but it's a great thing that aarch64 support came as an essentially free by-product. (And ARMv7+NEON will get automatically fixed onceany()andall()for ARMv7+NEON in thesimdcrate are fixed. Once we get Ion support for aarch64 in SpiderMonkey, we might actually ship the aarch64 code.)petrochenkov commentedon Jan 24, 2018
Sorry for a bikesheddy question (and it probably was discussed before), but why is the crate named "simd" and not "vendor_intrinsics" or something, if it's supposed to give access to intrinsics in general, not just SIMD-related ones, especially given that interfaces are not even
uNxMbased now.(This is kinda similar to the
gcccrate that provided access to all C/C++ compilers.)BurntSushi commentedon Jan 24, 2018
@petrochenkov Because the purpose of the crate has evolved independently of the name. When I first started this, the intent was to provide both vendor intrinsics and a minimal platform independent SIMD API. Given the evolution of purpose, a name change is likely a good idea.
gnzlbg commentedon Jan 24, 2018
hsivonen commentedon Jan 24, 2018
Like I said, I now understand the rationale for the approach even though I'm not happy about it.
Obviously, researching the lane configuration for everything is a big task, so there's the matter of prioritization: Making maximal intrinsic exposure the priority or making it the priority to do the commonality (i.e. 128-bit vectors) really well typing-wise up front. At this point, I don't think it productive for me to oppose the prioritization that has been chosen.
So we could have a subset (vendor-specific methods removed) of the
simdcrate on stable if I just wrote it up in RFC form?scottlamb commentedon Jan 24, 2018
There's something I'm missing in this discussion. If the vendor intrinsics are stabilized (using the vendor-specific types), can't the
simdcrate be rebuilt on top of those with its current abstractions? why would it still require nightly?hdevalence commentedon Jan 24, 2018
@scottlamb The reason is because the vendor intrinsics are themselves already implemented in terms of LLVM's vector types. For instance, the
_mm256_shuffle_epi32intrinsic is implemented in terms of an architecture-independent LLVM shuffle, which eventually lowers to the desiredvpshufdinstruction.alexcrichton commentedon Jan 25, 2018
@hdevalence
Ok cool, thanks for looking over the discussion!
@hsivonen
Thanks for the thoughts! I'd at least personally still like to have some discussion of the portable types before we stabilize anything to see how they fit in the bigger picture and all, but for me at least all the vendor intrinsics are currently in the state that they "should" be in (vendor types, defined by Intel, automatically verified, etc).
I think I may be less certain than @gnzlbg about an RFC here (but it probably can't hurt!). Would y'all perhaps be up for discussion on an issue solely focused around "What would a first pass of stabilization look like for portable types?" The motivation you oulined @hsivonen with running a risk of commonly being "accidentally less portable" is pretty convincing to me in terms of considering at least a minimal-ish subset of APIs for stabilization. (that being said, I'm also still not certain how we'd want to schedule that with stabilizing the vendor intrinsics, but there's time yet still!).
Although this may also be left to in-person discussion amongst stakeholders rather than an issue. I hope to do that when we're a little closer with the technical hurdles taken care of. So maybe not an issue just yet but a discussion soon!
@scottlamb
The
simdcrate uses different underlying infrastructure in LLVM to auto-magically get the best implementation based on the compilation target. While it's possible that we could replicate such logic in a user-defined crate I believe the point has been brought up historically that it's a huge amount of work (and not necessarily possible). The real crucial feature ofsimdis that it's built with compiler-aware knowledge of what's going on so optimizations can happen under the hood and whatnot.alexcrichton commentedon Jan 25, 2018
I've submitted to rust-lang/rust#47743 to solve the shim/ABI problem we've been experiencing. We're... edging ever closer to removing blockers for stablization!
gnzlbg commentedon Jan 28, 2018
@hsivonen As I see it, the path that has the most chances of success is:
Once we have these two things stable, re-implementing the mid-level
simdcrate and higher-level libraries likefastershould be possible in stable Rust (or we should at least be very close to that).In particular, the author of
fasterhas expressed interest in helping pushing for a strongly-typed SIMD layer around the weakly-typed vendor intrinsics, that higher-level SIMD libraries likefastercan build upon. This mid-level layer would probably look a lot like thesimdcrate and/or thestdsimdcrate version 0.0.4 and maybe such a layer makes it intostdsomeday through the RFC process.If someone is interested in working on this, I think that writing a single monolithic RFC for it right now is not the best idea because that RFC would be huge and mix way to many controversial and "in progress" things so progress and discussion will be very slow.
OTOH writing an RFC / crate proposing an API for portable vector types and their API (e.g. boolean types, reductions, conversions, etc.) is something that I think we want to stabilize soonish anyways, can be done right now in parallel to all other efforts, and something that any future SIMD mid-layer can build upon. The
stdsimdcrate already implements some of this, and so does thesimdcrate.hsivonen commentedon Jan 28, 2018
Why re-implementing? Would it not be OK to put the existing
simdcrate code--without thex86,armandaarch64modules exposed publicly--undercorewithout re-implementing it (i.e. letting it useplatform_intrinsicsinstead the vendor intrinsics for stuff likesimd_add)?gnzlbg commentedon Jan 28, 2018
You can try to write an RFC for putting the
simdcrate incorewithout providing an implementation of it that works in stable Rust. However, under the assumption that by that point such a stable implementation of thesimdcrate will be possible, I'd expect reasonable people to want to see such an implementation during the RFC process. At that point, why write an RFC at all? It's a lot of work (more work than just making thesimdcrate work on stable) and it does not really add any value.alexcrichton commentedon Feb 11, 2018
Ok! I'm going to close this now in favor of rust-lang/rfcs#2325, but thanks for the discussion everyone!