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

aarch64 neon intrinsics broken after #90621 #95002

Closed
hkratz opened this issue Mar 16, 2022 · 1 comment · Fixed by #91608
Closed

aarch64 neon intrinsics broken after #90621 #95002

hkratz opened this issue Mar 16, 2022 · 1 comment · Fixed by #91608
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) C-bug Category: This is a bug. O-AArch64 Armv8-A or later processors in AArch64 mode regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hkratz
Copy link
Contributor

hkratz commented Mar 16, 2022

#90621 causes a compile-time error on every aarch64 function which just has the neon feature explicitly enabled via #[target_feature(enable = "neon"), even if fp is enabled implicitly. This will cause a good amount of breakage across the ecosystem - Github Code Search. After this change even running TARGET=aarch64-apple-darwin cargo test on stdarch fails with lots of compile-time errors.

Apparently the stdarch build problems also cause any usage of aarch64 intrinsics to fail in current nightly. Trying to compile this:

fn main() {
    unsafe {
        let _ = core::arch::aarch64::vmovq_n_u8(0);
    }
}

results in:

$ cargo +nightly build
   Compiling xx v0.1.0 (/Users/hans/dev/xx)
error: the target features fp, neon must all be either enabled or disabled together
    --> /Users/hans/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/arm_shared/neon/mod.rs:5191:1
     |
5191 | #[target_feature(enable = "neon")]
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: add the missing features in a `target_feature` attribute

error: the target features fp, neon must all be either enabled or disabled together
    --> /Users/hans/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/arm_shared/neon/mod.rs:5721:1
     |
5721 | #[target_feature(enable = "neon")]
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: add the missing features in a `target_feature` attribute

error: could not compile `xx` due to 2 previous errors

(I found out because of a sudden CI failure for simdutf8 here.)

Originally posted by @hkratz in #90621 (comment)

@rustbot label O-arm A-simd C-bug

@rustbot rustbot added A-SIMD Area: SIMD (Single Instruction Multiple Data) O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state C-bug Category: This is a bug. labels Mar 16, 2022
@adamgemmell
Copy link
Contributor

Working on a fix for this.

unageek added a commit to unageek/graphest that referenced this issue Mar 17, 2022
@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Mar 17, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 17, 2022
@workingjubilee workingjubilee added O-AArch64 Armv8-A or later processors in AArch64 mode and removed O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state labels Mar 18, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 22, 2022
Fold aarch64 feature +fp into +neon

Arm's FEAT_FP and Feat_AdvSIMD describe the same thing on AArch64:
The Neon unit, which handles both floating point and SIMD instructions.
Moreover, a configuration for AArch64 must include both or neither.
Arm says "entirely proprietary" toolchains may omit floating point:
https://developer.arm.com/documentation/102374/0101/Data-processing---floating-point
In the Programmer's Guide for Armv8-A, Arm says AArch64 can have
both FP and Neon or neither in custom implementations:
https://developer.arm.com/documentation/den0024/a/AArch64-Floating-point-and-NEON

In "Bare metal boot code for Armv8-A", enabling Neon and FP
is just disabling the same trap flag:
https://developer.arm.com/documentation/dai0527/a

In an unlikely future where "Neon and FP" become unrelated,
we can add "[+-]fp" as its own feature flag.
Until then, we can simplify programming with Rust on AArch64 by
folding both into "[+-]neon", which is valid as it supersets both.

"[+-]neon" is retained for niche uses such as firmware, kernels,
"I just hate floats", and so on.

I am... pretty sure no one is relying on this.

An argument could be made that, as we are not an "entirely proprietary" toolchain, we should not support AArch64 without floats at all. I think that's a bit excessive. However, I want to recognize the intent: programming for AArch64 should be simplified where possible. For x86-64, programmers regularly set up illegal feature configurations because it's hard to understand them, see rust-lang#89586. And per the above notes, plus the discussion in rust-lang#86941, there should be no real use cases for leaving these features split: the two should in fact always go together.

Fixes rust-lang#95002.
Fixes rust-lang#95122.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Mar 22, 2022
@bors bors closed this as completed in 67d6cc6 Mar 23, 2022
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) C-bug Category: This is a bug. O-AArch64 Armv8-A or later processors in AArch64 mode regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants