reject aarch64 target feature toggling that would change the float ABI #133417

RalfJung · 2024-11-24T11:38:24Z

~~Stacked on top of #133099. Only the last two commits are new.~~

The first new commit lays the groundwork for separately controlling whether a feature may be enabled or disabled. The second commit uses that to make it illegal to disable the neon feature (which is only possible via -Ctarget-feature, and so the new check just adds a warning). Enabling the neon feature remains allowed on targets that don't disable neon or fp-armv8, which is all our built-in targets. This way, the entire PR is not a breaking change.

Fixes #131058 for hardfloat targets (together with #133102 which fixed it for softfloat targets).

Part of #116344.

rustbot · 2024-11-24T11:38:32Z

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-11-24T11:38:34Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

These commits modify compiler targets.
(See the Target Tier Policy.)

RalfJung · 2024-11-24T11:40:20Z

compiler/rustc_target/src/target_features.rs

+                    && !target.has_neg_feature("fp-armv8")
+                    && !target.has_neg_feature("neon")
+                {
+                    // neon is enabled by default, and has not been disabled, so enabling it again


I don't know where LLVM sets neon by default, so this claim is based on hearsay.

RalfJung · 2024-11-24T13:35:07Z

tests/ui/target-feature/feature-hierarchy.aarch64-sve2.stderr

oh interesting, this is triggered in an existing test...

So what happens here is that the flag is -Ctarget-feature=-neon,+sve2, and sve2 implies neon. So it makes little sense to say -neon here. I think it's fine to show a warning (and eventually hard error) for that.

The alternative would be to have a completely different approach for this entire "forbidden features" thing, where we look at the final state of all features, not at the individual enabled/disabled features. But that seems a lot more complicated and requires hooking deeper into the backend.

bors · 2024-11-28T14:21:19Z

☔ The latest upstream changes (presumably #133568) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-12-03T04:13:55Z

☔ The latest upstream changes (presumably #133770) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2024-12-13T16:29:57Z

#133099 got approved, so I rebased this and it should be ready for review @compiler-errors.

Or @workingjubilee do you want to take this?

compiler-errors · 2024-12-13T16:32:09Z

r? workingjubilee or reassign to me if you don't want to review it

…target feature is allowed

workingjubilee

slightly nitty. seems fine, small question

compiler/rustc_target/src/target_features.rs

workingjubilee · 2024-12-15T08:02:04Z

compiler/rustc_target/src/target_features.rs

+                    && !target.has_neg_feature("fp-armv8")
+                    && !target.has_neg_feature("neon")


these will only show up due to the target spec json, right?

They could also show up in future builtin targets. I want to future-proof against that since I can't expect everyone adding builtin targets to be aware of all the subtleties here.

compiler/rustc_target/src/spec/mod.rs

workingjubilee · 2024-12-15T09:35:12Z

re: is_some_and that is also nicer! thank you.

I obviously have a bad(?) case of combinator-brain.

workingjubilee

anyways the exact placement of such a remark can be deferred if you think otherwise, so!

r=me with the comment or not

Co-authored-by: Jubilee <workingjubilee@gmail.com>

RalfJung · 2024-12-15T10:23:41Z

I have moved the comment to the features field itself.

@bors r=workingjubilee

bors · 2024-12-15T10:23:45Z

📌 Commit 74e2ac4 has been approved by workingjubilee

It is now in the queue for this repository.

rust-timer · 2024-12-15T20:29:59Z

Finished benchmarking commit (d185062): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.3%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.9%, secondary -2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-4.6%, -1.3%]	2
Improvements ✅ (secondary)	-2.5%	[-3.0%, -2.0%]	2
All ❌✅ (primary)	-2.9%	[-4.6%, -1.3%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.875s -> 770.653s (0.10%)
Artifact size: 333.19 MiB -> 333.19 MiB (-0.00%)

rustbot assigned compiler-errors Nov 24, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 24, 2024

RalfJung commented Nov 24, 2024

View reviewed changes

RalfJung force-pushed the aarch64-float-abi branch from 0fb4919 to 42db03b Compare November 24, 2024 13:27

RalfJung commented Nov 24, 2024

View reviewed changes

RalfJung force-pushed the aarch64-float-abi branch from 42db03b to 6cd9e62 Compare November 28, 2024 21:27

RalfJung force-pushed the aarch64-float-abi branch from 6cd9e62 to e9967b9 Compare December 13, 2024 16:28

rustbot assigned workingjubilee and unassigned compiler-errors Dec 13, 2024

RalfJung added 2 commits December 14, 2024 08:24

target_features: control separately whether enabling and disabling a …

eb2e928

…target feature is allowed

reject aarch64 target feature toggling that would change the float ABI

1f8236d

RalfJung force-pushed the aarch64-float-abi branch from e9967b9 to 1f8236d Compare December 14, 2024 07:24

workingjubilee self-requested a review December 15, 2024 07:37

workingjubilee reviewed Dec 15, 2024

View reviewed changes

apply review feedback

0c9d42c

workingjubilee approved these changes Dec 15, 2024

View reviewed changes

RalfJung force-pushed the aarch64-float-abi branch from 8094e84 to 35b0e35 Compare December 15, 2024 10:22

advice against negative features in target specs

74e2ac4

Co-authored-by: Jubilee <workingjubilee@gmail.com>

RalfJung force-pushed the aarch64-float-abi branch from 35b0e35 to 74e2ac4 Compare December 15, 2024 10:23

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 15, 2024

bors merged commit d185062 into rust-lang:master Dec 15, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 15, 2024

RalfJung deleted the aarch64-float-abi branch December 15, 2024 21:58

This was referenced Dec 18, 2024

Fix the bug in CMPINT intrinsics with IMM3=7 rust-lang/stdarch#1694

Merged

Rustdoc failure in armv7 targets with -neon with target_feature(enable = "neon") #134511

Closed

RalfJung mentioned this pull request Dec 26, 2024

Add a notion of "some ABIs require certain target features" #134794

Merged

sayantn mentioned this pull request Feb 9, 2025

Rustdoc error in armv7 -neon with target_feature fp-armv8 #136755

Closed

ehuss mentioned this pull request Feb 21, 2025

rustdoc fails when building rust 1.85.0 for ARM targets #137366

Closed

DaniPopes mentioned this pull request Jul 22, 2025

Niched option check not optimized out #144329

Open

Jamesbarford mentioned this pull request Nov 20, 2025

Neon types generate bad code if the "neon" target feature is disabled. #118249

Closed

		&& !target.has_neg_feature("fp-armv8")
		&& !target.has_neg_feature("neon")

Uh oh!

reject aarch64 target feature toggling that would change the float ABI #133417

reject aarch64 target feature toggling that would change the float ABI #133417

Uh oh!

Conversation

RalfJung commented Nov 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 24, 2024

Uh oh!

rustbot commented Nov 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Nov 28, 2024

Uh oh!

bors commented Dec 3, 2024

Uh oh!

RalfJung commented Dec 13, 2024

Uh oh!

compiler-errors commented Dec 13, 2024

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

workingjubilee commented Dec 15, 2024

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Dec 15, 2024

Uh oh!

bors commented Dec 15, 2024

Uh oh!

Uh oh!

rust-timer commented Dec 15, 2024

Overall result: ✅ improvements - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RalfJung commented Nov 24, 2024 •

edited

Loading