Support for neon on stable rust

[HEnquist]

An early PR to show what I had in mind. Needs some cleanup, and updating of docs.

[HEnquist]

I just updated the ci to to a matrix test also for aarch64, with minimal rustc versions 1.61 with neon, 1.37 without. This of course fails at the moment since 1.61 hasn't been released yet. But once 1.61 is released it should be fine.

[HEnquist]

This is close to ready. Just two things left:

• The readme and the docs in lib.rs have diverged a bit, should we try to bring them back in sync?
• Tests on rust 1.37 on arm fails because cross doesn't support that version any longer: https://github.com/cross-rs/cross#minimum-supported-rust-version-msrv Ok to skip testing 1.37 on arm?

[ejmahler]

Nice! I will take a look.

Regarding the test failure, I honestly wouldn't be too torn up about just saying that msrv on arm is 1.61 or whatever version we're targeting for neon.

Re: docs and readme diverging, imo the readme should be a quick-skim version of what's present in docs.rs. Core features of the library, usage example, crate features, MSRV. And maybe leave the 4-to-5 upgrade guide. I can make an update to that once this PR is in, before I put out the 6.1.0 release

[HEnquist]

Just had another idea. When building on arm without the neon feature, there is nothing arm-specific in the code. Same as when building on x86_64 without both the AVX and SSE features. How about keeping 1.37 for non-neon arm and adding a CI job with --no-default-features?

[ejmahler]

That’s a great idea, and it’ll add some guaranteed coverage of the scalar path

On Tue, Jun 28, 2022 at 3:12 PM Henrik Enquist @.***> wrote:

Just had another idea. When building on arm without the neon feature, there is nothing arm-specific in the code. Same as when building on x86_64 without both the AVX and SSE features. How about keeping 1.37 for non-neon arm and adding a CI job with --no-default-features?

— Reply to this email directly, view it on GitHub https://github.com/ejmahler/RustFFT/pull/84#issuecomment-1169334685, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAI2M6TFHEHBQ7QAGTT2DDDVRN2DZANCNFSM5S2SBG6Q . You are receiving this because you commented.Message ID: @.***>

[HEnquist]

Updated the ci!

BTW I got help to run the benches on an Apple M1. It performs quite well with neon. With and without neon:

Compared to my old Ryzen:

[ejmahler]

Ok, gonna be taking a look at this now

[ejmahler]

One question I have: Should we enable neon compilation by default? If we did, we'd be more consistent with the fact that the rest of them are enabled by default. I can picture users never realizing that they're leaving performance on the table by not knowing that there's a feature flag they can enable.

On the other hand, people using rusfft on arm might be annoyed if it suddenly stops compiling, and they have to go in and add a feature flag to get going again.

We're following the policy to the letter, so there shouldn't be any uproar, I'm just always squeamish about forcing people to change things on upgrade.

Maybe a good compromise is to enable it by default, but put "If the 'neon' feature flag is disabled, the MSRV is 1.37" directly into the build.rs error message, so that at the very least, anyone impacted won't have to do much research?

[HEnquist]

Hmm yes you are probably right, if neon isn't enabled by default many will run without it and not realize it. I like the idea of including the msrv hint in the build error message. The downside is that anyone building for many platforms with an older rustc will need to disable default features and add back avx and sse if they want to keep those.. I'm having a hard time deciding what I prefer :)

[HEnquist]

Sorry this took so long. I have enabled the neon feature by default, and updated build.rs to give better messages on panics.

Compiling on x86_64 with a too old rustc (faked by setting a dummy MSRV of 1.77):

   Compiling rustfft v6.0.1 (C:\Users\henrik\rust\fft\RustFFT)
error: failed to run custom build command for `rustfft v6.0.1 (C:\Users\henrik\rust\fft\RustFFT)`

Caused by:
  process didn't exit successfully: `C:\Users\henrik\rust\fft\RustFFT\target\debug\build\rustfft-20369f781dfd58e6\build-script-build` (exit code: 101)
  --- stdout
  cargo:rerun-if-changed=build.rs

  --- stderr
  thread 'main' panicked at '
  ====
  Unsupported rustc version 1.64.0
  RustFFT needs at least 1.77.0
  ====
  ', build.rs:14:24
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

And on aarch64 (again faked with a dummy MSRV):

   Compiling rustfft v6.0.1 (C:\Users\henrik\rust\fft\RustFFT)
error: failed to run custom build command for `rustfft v6.0.1 (C:\Users\henrik\rust\fft\RustFFT)`

Caused by:
  process didn't exit successfully: `C:\Users\henrik\rust\fft\RustFFT\target\debug\build\rustfft-20369f781dfd58e6\build-script-build` (exit code: 101)
  --- stdout
  cargo:rerun-if-changed=build.rs

  --- stderr
  thread 'main' panicked at '
  ====
  Error! Unsupported rustc version 1.64.0
  RustFFT with neon support needs at least 1.71.0
  If the 'neon' feature flag is disabled, the minimum version is 1.37.0
  ====
  ', build.rs:28:24
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[HEnquist]

Is there anything more to do here, or could we get this merged?

[ejmahler]

There's nothing in the way, I've just been a combination of busy and depressed that's led to me procrastinating on side projects. I will get to this on Monday.

[HEnquist]

No problem, don't worry about it! But let me know if there is anything I can do to help, like updating documentation for the next release or stuff like that.

[ejmahler]

https://users.rust-lang.org/t/release-rustfft-6-1-now-with-support-for-aarch64-neon-acceleration/83954