-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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-softfloat: forbid enabling the neon target feature #135160
base: master
Are you sure you want to change the base?
Conversation
r? @Noratrieb rustbot has assigned @Noratrieb. Use |
This change looks good to me. On the off chance that a kernel user comes up in the near future, they can use the "be careful" approach you mentioned in #134375 (comment). |
Yeah, separately building some crates with |
Will that mean something like |
I don't think we should allow target features to do that (I was imagining target modifiers would introduce a new flag for overriding ABI-relevant target features, if there's any good use for that), and so far there was no proposal to allow overriding target modifiers on a per-function basis. The text you quoted referred to |
2bd82d7
to
bdf1e78
Compare
I'm happy with this as long as we acknowledge that this is a temporary workaround until LLVM starts handling ABI independently of target features. |
Fully agreed, the ideal end state is that we can use the FPU while sticking to a softfloat ABI (what clang and GCC do for |
Was this discussed by T-lang prior to be approved? I assume yes since I see the release notes label, correct? cc @traviscross probably knows |
We are waiting for t-lang approval, hence the "I-lang-nominated" and "S-waiting-on-team" labels. |
This fixes #134375 in a rather crude way, by making the example not build any more on aarch64-unknown-none-softfloat. That is a breaking change since the "neon" aarch64 target feature is stable, but this is justified as a soundness fix. Note that it's not "neon" which is problematic but "fp-armv8"; however, the two are tied together by rustc.
More work on the LLVM side will be needed before we can let people use neon without impacting the ABI of float values (and, in particular, the ABI used by automatically inserted calls to libm functions, e.g. for int-to-float casts, which rustc has no control over).
Nominating for @rust-lang/lang since it is a breaking change. As-is this PR doesn't have a warning cycle; the hope is that the aarch64-unknown-none-softfloat target is sufficiently niche that there's no huge fallout and we can easily revert if it causes trouble. A warning cycle could be added but would need some dedicated rather hacky check in the target_feature attribute handling logic.