add conditional checking of neon

[chris-ha458]

Target attribute aarch64
and
std::arch::is_aarch64_feature_detected
seem to be stabilised now
rust-lang/rust#86941
rust-lang/rust#90620

Would this be a good idea now?
If so, how could I test this?

[chris-ha458]

found a way to cross compile but it seems like much more changes would be required than a simple if change.
As documented, code was written under the assumption that it aarch64 implies neon(much like x86 implies either avx or sse) so it might be difficult to decouple that.

[chris-ha458]

This is implemented in compile time only.
There might be a way to implement it as was done in avx vs sse4.1 but hopefully it will be the next step.

[nickbabcock]

Thanks for the contribution this sounds reasonable. Couple of quick thoughts before looking deeper into this:

• This bumps the MRSV to 1.60, right?
• To test: one can disable neon with RUSTFLAGS="-C target-feature=-neon" like this:

  RUSTFLAGS="-C target-feature=-neon" cross test --target aarch64-unknown-linux-gnu

Looks like there are errors that will need to be fixed up.

[chris-ha458]

This bumps the MRSV to 1.60, right?

Due to is_aarch64_feature_detected that seems so to me.
cargo msrv confirms this.
Finished The MSRV is: 1.60.0

test: one can disable neon with RUSTFLAGS="-C target-feature=-neon" like this:

I tried this, (including installing cross) but it is not working atm under my environment. I'll investigate that in the future.

[chris-ha458]

I think the nature of the errors mean that I can atleast try to fix them with
RUSTFLAGS="-C target-feature=-neon" cargo clippy --target aarch64-unknown-linux-gnu
(of course runtime tests would require an actual build setup)

[chris-ha458]

I've been checking the errors and fixing them with clearer conditional compilation settings. I get and fundamentally there is an issue due to this :
https://github.com/chris-ha458/highway-rs/tree/runtime_attempt
it seems like there is not a clean way to make this runtime detect compatible.
AFAICT the union types need to be known during compile time,
and with
RUSTFLAGS="-C target-feature=-neon" cargo clippy --target aarch64-unknown-linux-gnu
It is not possible to add a runtime conditional check that could make this compile.
Am I missing anything?

union HighwayChoices {
    #[cfg(not(any(
        all(target_family = "wasm", target_feature = "simd128"),
        target_arch = "aarch64"
    )))]
    portable: ManuallyDrop<PortableHash>,
    #[cfg(target_arch = "x86_64")]
    avx: ManuallyDrop<AvxHash>,
    #[cfg(target_arch = "x86_64")]
    sse: ManuallyDrop<SseHash>,
    #[cfg(target_arch = "aarch64")]
    neon: ManuallyDrop<NeonHash>,
    #[cfg(target_arch = "aarch64")]
    portable: ManuallyDrop<PortableHash>,
    #[cfg(all(target_family = "wasm", target_feature = "simd128"))]
    wasm: ManuallyDrop<WasmHash>,
}

[nickbabcock]

It should be possible, see the x86 code for inspiration.

You can see the difference when benchmarking:

RUSTFLAGS="-C target-feature=-sse4.1,-avx2" cargo bench --no-default-features -- 64bit/builder/256

# vs.

RUSTFLAGS="-C target-feature=+sse4.1,-avx2" cargo bench --no-default-features -- 64bit/builder/256

There is also CI code that makes sure it can compile:

highway-rs/.github/workflows/main.yml

Line 142 in cf11588

feature: ["-sse4.1,-avx2", "-sse4.1,+avx2", "+sse4.1,+avx2", "+sse4.1,-avx2"]

[chris-ha458]

I think I misunderstood the union data structure.
I fixed it now, and
RUSTFLAGS="-C target-feature=-neon" cargo build --target aarch64-unknown-linux-gnu
and cargo clippy of the above and --release all pass.
(clippy does complain of clippy::missing_safety_doc though).

Next in line would be to setup the runtime tests which would require cargo cross

[nickbabcock]

Thanks, some more thoughts before committing myself to this:

Are you able to provide a sample use case for the conditional neon check? If neon is ubiquitous, supporting this use case may be more effort than it is worth as now aarch64 users need to either opt into providing RUSTFLAGS="-C target-feature=+neon" or use the conditional check to get the neon implementation, and I want to make sure this is worth it.

From an earlier discussion: #51 (comment)

Armv8 specification requires all compliant processors to implement NEON and FP support

It seems like assuming neon support is relatively safe.

[chris-ha458]

It seems like assuming neon support is relatively safe.

tbh I agree with this.
My opinion is that we could do away with this PR and actually going the otherway and removing all neon related checks at all.
Just assume Armv8 equals neon support and remove all separate compile and runtime checks that would separate such.

However, I am unable to make that claim definitely especially towards a repo that I am not a maintainer of, so I went the other way around and rather implemented the neon checks to parity with the x86 checks.

I see three paths forward.

1. Merge this PR with or without further testing
2. Close this PR and leave armv8 related code as is going forward.
3. Open a new PR that simplifies the code under the assumption that Armv8 == neon

As mentioned previously I actually prefer 3 over all else since in the long run it would reduce total code and ongoing code maintenance.

[nickbabcock]

Apologies, I meant to respond before going on holiday for a week. I'm not sure I quite understand the third option, afaik neon is already assumed when target_arch is aarch64.

[chris-ha458]

highway-rs/src/builder.rs

Line 162 in cf11588

// Based on discussions here:

My proposed changes would be to add change the documentation here, to reflect that
the current approach is intended and not due to certain features not being stable yet.
For instance, std::is_aarch64_feature_detected is stable now, and neon support can be detected either compile time or runtime but it is NOT implemented since there is no reason to believe aarch64 to be separate from neon support at this time.

[nickbabcock]

Oh yes, of course. Good eye. Yeah that comment can be updated, as I agree that assuming neon is present is a safe bet.