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.

Sign up for GitHub

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

Jump to bottom

AArch64 target feature cfgs are being incorrectly enabled #95122

Closed
Amanieu opened this issue Mar 19, 2022 · 4 comments · Fixed by #91608
Closed

AArch64 target feature cfgs are being incorrectly enabled #95122

Amanieu opened this issue Mar 19, 2022 · 4 comments · Fixed by #91608
Labels
C-bug Category: This is a bug. O-AArch64 Armv8-A or later processors in AArch64 mode T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Amanieu
Copy link
Member

Amanieu commented Mar 19, 2022 •
edited
Loading

This seems to have been introduced in nightly 2022-03-16 which includes #90621.

Previous correct behavior:

# rustc +nightly-2022-03-15 --print cfg --target aarch64-unknown-linux-gnu | grep target_feature
target_feature="fp"
target_feature="llvm14-builtins-abi"
target_feature="neon"
target_feature="pmuv3"

New incorrect behavior:

# rustc +nightly-2022-03-16 --print cfg --target aarch64-unknown-linux-gnu | grep target_feature
target_feature="f32mm"
target_feature="f64mm"
target_feature="fhm"
target_feature="fp"
target_feature="fp16"
target_feature="jsconv"
target_feature="llvm14-builtins-abi"
target_feature="neon"
target_feature="pmuv3"
target_feature="sve"
target_feature="sve2"
target_feature="sve2-aes"
target_feature="sve2-bitperm"
target_feature="sve2-sha3"
target_feature="sve2-sm4"

cc @adamgemmell
@Amanieu Amanieu added C-bug Category: This is a bug. O-AArch64 Armv8-A or later processors in AArch64 mode labels Mar 19, 2022
@workingjubilee
Copy link
Contributor

Commenting out the match arm in to_llvm_features for a given mapping alters the emitted features:

        // Rust ties fp and neon together. In LLVM neon implicitly enables fp,
        // but we manually enable neon when a feature only implicitly enables fp
        ("aarch64", "f32mm") => smallvec!["f32mm", "neon"],
        // ("aarch64", "f64mm") => smallvec!["f64mm", "neon"],
        ("aarch64", "fhm") => smallvec!["fp16fml", "neon"],
        // ("aarch64", "fp16") => smallvec!["fullfp16", "neon"],
        ("aarch64", "jsconv") => smallvec!["jsconv", "neon"],
        // ("aarch64", "sve") => smallvec!["sve", "neon"],
        ("aarch64", "sve2") => smallvec!["sve2", "neon"],
        // ("aarch64", "sve2-aes") => smallvec!["sve2-aes", "neon"],
        ("aarch64", "sve2-sm4") => smallvec!["sve2-sm4", "neon"],
        // ("aarch64", "sve2-sha3") => smallvec!["sve2-sha3", "neon"],
        ("aarch64", "sve2-bitperm") => smallvec!["sve2-bitperm", "neon"],
        (_, s) => smallvec![s],

Result is:

$ rustc +stage1 --print cfg --target aarch64-unknown-linux-gnu | grep target_feature
'+fp16' is not a recognized feature for this target (ignoring feature)
'+fp16' is not a recognized feature for this target (ignoring feature)
target_feature="f32mm"
target_feature="fhm"
target_feature="fp16"
target_feature="jsconv"
target_feature="llvm14-builtins-abi"
target_feature="neon"
target_feature="pmuv3"
target_feature="sve2"
target_feature="sve2-bitperm"
target_feature="sve2-sm4"

@workingjubilee workingjubilee mentioned this issue Mar 20, 2022
Merged
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 22, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 22, 2022
@Dylan-DPC
Rollup merge of rust-lang#91608 - workingjubilee:fold-neon-fp, r=Amanieu
e77d593 
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
@workingjubilee
Test SVE2 includes NEON and not vice versa
8fa4ae8 
Required by rust-lang#95122
@workingjubilee workingjubilee mentioned this issue Mar 23, 2022
Closed
@bors bors closed this as completed in 67d6cc6 Mar 23, 2022
@pthariensflame
Copy link
Contributor

@workingjubilee Should I open a separate issue for the pmuv3 part?

@workingjubilee
Copy link
Contributor

workingjubilee commented Mar 24, 2022 •
edited
Loading

That seems to have been a preexisting feature but yes, if you believe it is in error then feel free to open a new issue about it.

EDIT: Ah, yeah, I think our target for darwin isn't "an M1", in actuality, and that's what's causing conflict.

@pthariensflame pthariensflame mentioned this issue Mar 25, 2022
Open
@pthariensflame
Copy link
Contributor

@workingjubilee Alright; I've opened #95319.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: This is a bug. O-AArch64 Armv8-A or later processors in AArch64 mode T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fold aarch64 feature +fp into +neon workingjubilee/rustc
4 participants
@pnkfelix @Amanieu @pthariensflame @workingjubilee