-
Notifications
You must be signed in to change notification settings - Fork 464
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
Support SIMD on Rust stable #520
Conversation
Awesome! |
After this PR gets in I also have another PR in queue (not yet finished, but works already) where I want to switch SIMD to be entirely autodetected at runtime so that it's not necessary to compile with |
Runtime autodetection sounds great! |
Can I ask that you use either use |
Yes, I will still keep the ability to override/disable this; I'd just like autodetection to be the default. |
FWIW, I have a PR open to add AVX-512 support to |
(Rebased to resolve the conflict in |
If I can interject in the conversation, I have a small question. Knowing If that's a trivial change, that would allow WASM ed25519 to benefit from WASM basically for free. If it's not, then nevermind. Edit: After looking at the PR, it seems x86_64 intrinsics are directly called so this isn't a trivial change. I guess this is for another PR? |
All of the current SIMD backends use explicit architecture-specific compiler intrinsics, so that wouldn't do anything besides break compilation when targetting WASM. The current dependency on You'd basically have to add a WASM-specific backend. |
It's also important that whatever operations are used remain constant-time, which needs to be verified for every platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, and with hardly any changes! I left some commends, mostly in the packed_simd.rs
file. I'm not super familiar with these intrinsics so some questions might be obvious.
@tarcieri looks good to go, modulo CI and readme updates to reflect that nightly isn't needed for AVX2 anymore. Also I was looking for a source on why the AVX2 add, sub, mul, shl, and shr functions we use are constant time, but I got nothing. |
Do you want me to also update those?
I can't comment on constant time-ness of the rest of the code existing code as I haven't analyzed it in that regard, but AFAIK all of these should be constant time. Although this does kind of depend on the microarchitectural details of the CPU on which we're running I think all of modern Intel and AMD CPUs should have all of these implemented as constant time, (Also, please note that this PR doesn't really change much here; these were still used before, just indirectly through the |
Re docs: that'd be great actually! If you don't have time, one of us will do it, probably this week. Re constant time: Yup, agreed, and it seems that every serious curve25519 impl uses AVX2. It would just be nice to be able to point to something that justifies our use of it. |
I've updated the README and I've also added a job to the CI to test this. (Hopefully it'll work?) Initially I only added a job to build the code on Rust stable, but then I realized that these tests are not being run at all, and we should be able to run them, at least for AVX2 (which at this point is 10 years old, so it's a pretty safe bet to assume that the test runners should support it). |
They're pretty routinely used for cryptography. I would be fairly surprised to learn they have any data dependent timing variability, which I wouldn't expect from any arithmetic instructions on modern CPUs whether they're SIMD or not. (They can, however, have timing variability based on thermal throttling, but that's a whole different can of worms) |
@tarcieri good to merge? This might also require an MSRV bump right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I don't think it needs an MSRV bump? AVX (<512) intrinsics have been stable for quite awhile.
Maybe there could be an MSRV test with +avx2
enabled? But really the answer there is runtime feature gating as @koute mentioned.
Oh ok. I just assumed this wouldn't work with anything prior to the version that stabilized AVX2. |
Oh I was under the impression that stable AVX2 was new. Apparently it was stabilized in 2018. I'll try to consolidate the test to run both builds in MSRV. Also, I just ran AVX2 tests on my x86_64 machine and everything passess. |
Thanks again @koute! This is a great addition |
Thanks! |
To anyone who was following this PR, my followup PR with runtime autodetection is now up: #523 |
This PR makes the SIMD backend work on stable Rust.
packed_simd
dependency was removed.packed_simd
was added.