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.

bors · 2024-12-15T16:32:07Z

⌛ Testing commit 74e2ac4 with merge d185062...

bors · 2024-12-15T19:13:16Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing d185062 to master...

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%)

…workingjubilee reject unsound toggling of RISCV target features ~~Stacked on top of rust-lang#133417, only the last commit is new.~~ Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment))) Part of rust-lang#116344 Cc `@beetrees` I hope I got everything. I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks. r? `@workingjubilee` Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.

…workingjubilee reject unsound toggling of RISCV target features ~~Stacked on top of rust-lang#133417, only the last commit is new.~~ Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment))) Part of rust-lang#116344 Cc ``@beetrees`` I hope I got everything. I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks. r? ``@workingjubilee`` Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.

Rollup merge of rust-lang#134337 - RalfJung:riscv-target-features, r=workingjubilee reject unsound toggling of RISCV target features ~~Stacked on top of rust-lang#133417, only the last commit is new.~~ Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment))) Part of rust-lang#116344 Cc ``@beetrees`` I hope I got everything. I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks. r? ``@workingjubilee`` Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.

…s, r=workingjubilee Add a notion of "some ABIs require certain target features" I think I finally found the right shape for the data and checks that I recently added in rust-lang#133099, rust-lang#133417, rust-lang#134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both `-Ctarget-feature` and `#[target_feature]` are updated to ensure we follow the rules of the ABI. This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features. We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this *before* applying `-Ctarget-feature` to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested. For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;) As a side-effect this also (unstably) allows *enabling* `x87` when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see rust-lang#133611, but no such change is happening in this PR. r? `@workingjubilee`

…ingjubilee Add a notion of "some ABIs require certain target features" I think I finally found the right shape for the data and checks that I recently added in rust-lang/rust#133099, rust-lang/rust#133417, rust-lang/rust#134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both `-Ctarget-feature` and `#[target_feature]` are updated to ensure we follow the rules of the ABI. This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features. We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this *before* applying `-Ctarget-feature` to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested. For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;) As a side-effect this also (unstably) allows *enabling* `x87` when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see rust-lang/rust#133611, but no such change is happening in this PR. r? `@workingjubilee`

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 S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 15, 2024

RalfJung mentioned this pull request Dec 15, 2024

reject unsound toggling of RISCV target features #134337

Merged

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

Open

RalfJung mentioned this pull request Dec 26, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RalfJung commented Nov 24, 2024 •

edited

Loading

rustbot commented Nov 24, 2024

rustbot commented Nov 24, 2024

RalfJung Nov 24, 2024

RalfJung Nov 24, 2024

RalfJung Nov 24, 2024

bors commented Nov 28, 2024

bors commented Dec 3, 2024

RalfJung commented Dec 13, 2024

compiler-errors commented Dec 13, 2024

workingjubilee left a comment

workingjubilee Dec 15, 2024

RalfJung Dec 15, 2024

workingjubilee commented Dec 15, 2024

workingjubilee left a comment

RalfJung commented Dec 15, 2024

bors commented Dec 15, 2024

bors commented Dec 15, 2024

bors commented Dec 15, 2024

rust-timer commented Dec 15, 2024

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

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

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

Conversation

RalfJung commented Nov 24, 2024 • edited Loading

rustbot commented Nov 24, 2024

rustbot commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 28, 2024

bors commented Dec 3, 2024

RalfJung commented Dec 13, 2024

compiler-errors commented Dec 13, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Dec 15, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

RalfJung commented Dec 15, 2024

bors commented Dec 15, 2024

bors commented Dec 15, 2024

bors commented Dec 15, 2024

rust-timer commented Dec 15, 2024

Overall result: ✅ improvements - no action needed

RalfJung commented Nov 24, 2024 •

edited

Loading