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

Put aarch64 simd feature behind nightly feature flag #284

Closed
wants to merge 1 commit into from

Conversation

rubdos
Copy link
Contributor

@rubdos rubdos commented May 1, 2021

Hi! We've just merged the libsignal-client swap in libsignal-service-rs. Everything went smooth, except for one thing! Whisperfish' aarch64 target does not compile on Rust stable; all others (armv7hl, i[46]86) work perfectly.

Since we have some other requirements w.r.t. Rust compiler versions (Tokio, mostly), I figured I could take a look on how to get libsignal-client to compile with stable Rust. Seemingly, the only real place where the nightly-1.49 compiler is required (for the Rust library, that is), is for the aarch64 SIMD optimizations.


This pull request models the simplest solution that I could come up with: expose a nightly flag from the affected crates (basically all crates), and only enable feature(stdsimd) when that feature is enabled.

I've also taken the liberty of linking that nightly flag to the nightly flag of the Dalek set of crates. This has a side effect: the Dalek-nightly flag was previously not enabled as far as I could tell. I've linked it through because I feel it would be more confusing if I did not, with this name.

Possible approaches

nightly flag (this pull request)

The approach in this pull request.

Pro

  • Simple

Con

  • Also enabled Dalek-nightly
  • aarch64-simd is now by default off, except if you set it default-on in ffi, which would then default-on Dalek-nightly too

Separate feature flag (e.g. aarch64simd)

The same idea, but separate flag

Pro

  • Relatively simple
  • Keeps dalek separate from aarch64

Con

  • More feature flags to maintain, more configurations to test

If you have more suggestions, I'll add them to the list. I have the current proposal in a separate branch that we can use for now.

@jrose-signal
Copy link
Contributor

Interesting, will have to think about this a bit first. We're already beholden to nightly because our use of cbindgen-with-macros (see #141) and our AArch64 SIMD is already written to do a runtime check. At first thought that steers me towards the aarch64simd flag that's included in the default features, even though that's not the typical way to do these things. But we are leaving performance on the table by not passing through Dalek-nightly, and we could easily make the nightly feature a requirement of libsignal-bridge-shared and leave it at that. I'll come back to this.

@jrose-signal
Copy link
Contributor

It looks like the nightly flag for curve25519-dalek doesn't really do anything related to codegen anymore; it only affects rustdoc. (We can't use the vector backends because they don't check for CPU support first; they're either on or off.) Because of that I think I'd pick your second approach of a dedicated aarch64simd flag that's activated by libsignal-bridge-shared, on the grounds that "it's very unconventional for a Rust crate's default features to require nightly" but also "if you want to use this from Swift or Java or TypeScript you have to compile the same way Signal does".

@rubdos
Copy link
Contributor Author

rubdos commented May 7, 2021

Interesting with regard to vector backends for Dalek; would a runtime detection feature be something that would interest Signal? I've always wanted to toy with a NEON backend for 25519 at some point, so maybe I should take out two flies with that stone.

I'll update this PR to take the second approach later, thanks for the feedback and for coming back to us!

@jrose-signal
Copy link
Contributor

Interesting with regard to vector backends for Dalek; would a runtime detection feature be something that would interest Signal? I've always wanted to toy with a NEON backend for 25519 at some point, so maybe I should take out two flies with that stone.

Hm. On the one hand, it's bad enough that we're diverging from upstream, but on the other we do a lot of Curve25519 operations. You can check out the AES implementation in signal-crypto to see how we're doing runtime detection of both NEON and AVX2 there.

@rubdos
Copy link
Contributor Author

rubdos commented May 14, 2021

(Have had a busy week, I'll still come back to this)

Hm. On the one hand, it's bad enough that we're diverging from upstream, but on the other we do a lot of Curve25519 operations. You can check out the AES implementation in signal-crypto to see how we're doing runtime detection of both NEON and AVX2 there.

I'd mostly be trying to get this upstream with Dalek: implement a NEON backend, and then get them to merge some runtime detection, potentially behind a feature flag. Diverging any more from upstream would be pretty painful, especially for these kind of things ;-)

@jrose-signal
Copy link
Contributor

#328 will make this job a lot easier! And I guess we should follow RustCrypto's lead in calling this armv8.

@rubdos
Copy link
Contributor Author

rubdos commented Jul 22, 2021

#328 will make this job a lot easier! And I guess we should follow RustCrypto's lead in calling this armv8.

I'm opening an new PR with the previous approach at CEST-tomorrow.

@rubdos rubdos closed this Jul 22, 2021
@rubdos rubdos deleted the make-simd-optional branch July 22, 2021 20:51
@rubdos rubdos restored the make-simd-optional branch April 9, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants