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

[CMake] Rework NEON detection #314

Conversation

BurningEnlightenment
Copy link
Collaborator

@BurningEnlightenment BurningEnlightenment commented Jun 12, 2023

Given the myriad of -mfpu options for ARM [1], the inability to portably query for CPU support, and the lack of standardized ISA names we have no other choice, but to opt out of automatically supplying NEON compile flags. Instead we simply add the NEON optimized source file if we detect an ISA with guaranteed NEON support (>= ARMv8) or the user explicitly requests it (in which case he is expected to provide the compile flags with CMAKE_C_FLAGS or BLAKE3_CFLAGS_NEON either through a toolchain file or commandline parameters).

Resolves #312

Given the myriad of `-mfpu` options for ARM [1], the inability to
portably query for CPU support, and the lack of standardized ISA names
we have no other choice, but to opt out of automatically supplying NEON
compile flags. Instead we simply add the NEON optimized source file if
we detect an ISA with guaranteed NEON support (>= ARMv8) or the user
explicitly requests it (in which case he is expected to provide the
compile flags with `CMAKE_C_FLAGS` or `BLAKE3_CFLAGS_NEON` either
through a toolchain file or commandline parameters).

[1]: https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
@oconnor663
Copy link
Member

oconnor663 commented Jun 17, 2023

This is the same thing we do in the Rust build and in blake3_impl.h, so it sounds like a good change to me.

Is there a reason we need both BLAKE3_USE_NEON and BLAKE3_USE_NEON_INTRINSICS? Could we unify them into just BLAKE3_USE_NEON, which is already in use?

@BurningEnlightenment
Copy link
Collaborator Author

Is there a reason we need both BLAKE3_USE_NEON and BLAKE3_USE_NEON_INTRINSICS? Could we unify them into just BLAKE3_USE_NEON, which is already in use?

The former is a C define, the latter is a CMake variable which is requesting to build with NEON intrinsic support (which may not be granted).

@oconnor663
Copy link
Member

The approach we've been using in other places is that the default is "try to be smart about using NEON" (mainly just use it on AArch64 I think), and then if BLAKE3_USE_NEON is set we interpret 1 as "always use NEON (and possibly trigger compiler errors but so be it)" or 0 as "never use NEON". I like that that keeps the behavior of our explicitly flags as simple as possible, because we're probably on the hook for keeping those vaguely stable :)

That said, I'm going to land this as-is, and I'll let you make the call on whether we should make any more changes.

@oconnor663 oconnor663 merged commit 3f396d2 into BLAKE3-team:master Jun 18, 2023
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aarch64: gcc: error: unrecognized command-line option '-mfpu=neon'
2 participants