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

RalfJung · 2024-12-26T18:23:53Z

I think I finally found the right shape for the data and checks that I recently added in #133099, #133417, #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 #133611, but no such change is happening in this PR.

r? @workingjubilee

rustbot · 2024-12-26T18:23:56Z

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

rustbot · 2024-12-26T18:24:01Z

r? @davidtwco

rustbot has assigned @davidtwco.
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-12-26T18:24:03Z

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

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

workingjubilee · 2024-12-26T22:13:08Z

I will get around to this, I just put a freeze on my queue because I was accumulating enough reviews in addition to ongoing patch series. :P

taiki-e · 2024-12-27T01:28:07Z

compiler/rustc_target/src/target_features.rs

+                        // Embedded ABI, requires e.
+                        (&["e"], &[])
+                    }
+                    "ilp32" | "lp64" => NOTHING,


Suggested change

"ilp32" | "lp64" => NOTHING,

"ilp32" | "lp64" => {

// Incompatible with e.

(&[], &["e"])

}

Ah right the old logic treated all the non-e ABIs the same, we should keep that.

The old logic also allowed disabling e when using an e ABI, so we should probably keep that, too.

RalfJung · 2024-12-27T14:57:49Z

@ojeda wrote

(currently, arm64 in the kernel uses --target=aarch64-unknown-none -Ctarget-feature="-neon").

That no longer works after this PR (neon is a required ABI feature so it cannot be disabled with -Ctarget-feature)... so I probably need to at least slightly adjust my strategy here.

RalfJung · 2024-12-27T20:40:04Z

compiler/rustc_target/src/target_features.rs

+                // We support 2 ABIs, hardfloat (default) and softfloat.
+                if self.has_feature("soft-float") {
+                    NOTHING


It seems like this might be wrong, and we should be checking self.abi instead... but I am not sure yet.

RalfJung · 2024-12-27T20:46:04Z

compiler/rustc_codegen_llvm/src/llvm_util.rs

+        for feature in abi_enable {
+            all_rust_features.push((true, feature));
+        }
+        for feature in abi_disable {
+            all_rust_features.push((false, feature));
+        }


This is the part where we explicitly set the ABI-relevant features. The concern here is on the one hand not knowing the default target features, and also not being entirely sure that we correctly model all "implied features" on the LLVM side.

Currently this adds the ABI-relevant features after the ones from -Ctarget-features, i.e. we are overriding the user here. That's a massive change in behavior. I feel I should probably swap the order, but then we are not protected against unexpected feature implications any more. Still, we probably want the target modifier story to be more solidified before we do any behavior changes here.

RalfJung · 2024-12-27T21:32:17Z

That no longer works after this PR (neon is a required ABI feature so it cannot be disabled with -Ctarget-feature)... so I probably need to at least slightly adjust my strategy here.

I changed this to add the ABI features before the user-defined features, so things will behave as before with -Ctarget-feature=-neon on ARM targets. However, a warning is shown since this is not a supported combination -- long-term we want to forbid disabling neon on targets that need float registers for their ABI.

tgross35 · 2024-12-27T23:02:57Z

I changed this to add the ABI features before the user-defined features, so things will behave as before with -Ctarget-feature=-neon on ARM targets. However, a warning is shown since this is not a supported combination -- long-term we want to forbid disabling neon on targets that need float registers for their ABI.

Could we instead add a -nofloat version of these targets? Or provide a compiler way to disable all use of float types with a build-std config to go with it.

bors · 2024-12-31T09:29:13Z

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

…ures

…led by the ABI target feature check

RalfJung · 2024-12-31T11:48:24Z

Rebased now that #134932 landed; the ARM code here now is very clean. We don't even have to mark soft-float as a "forbidden feature" any more. X86 is okay from a soundness perspective (by refusing to ever toggle the soft-float feature); aarch64 is a sad mess that I think we can only fix by refusing to toggle neon on our softfloat target -- this PR doesn't do that as I want this PR to not be a breaking change.

workingjubilee

thank you! this does look a lot nicer overall. only a small nit or three (as most of the "review" of "how even to proceed?" happened on Zulip already)

compiler/rustc_codegen_gcc/src/gcc_util.rs

compiler/rustc_target/src/target_features.rs

workingjubilee · 2025-01-05T21:15:56Z

Wonderful! @bors r+

bors · 2025-01-05T21:15:59Z

📌 Commit 2e64b53 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2025-01-05T23:21:09Z

⌛ Testing commit 2e64b53 with merge feb32c6...

bors · 2025-01-06T02:04:56Z

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

rust-timer · 2025-01-06T03:22:01Z

Finished benchmarking commit (feb32c6): comparison URL.

Overall result: ❌ regressions - 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.3%	[0.3%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -1.7%, secondary 0.6%)

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)	1.3%	[0.5%, 2.7%]	6
Improvements ✅ (primary)	-1.7%	[-2.3%, -1.2%]	8
Improvements ✅ (secondary)	-1.7%	[-1.7%, -1.7%]	2
All ❌✅ (primary)	-1.7%	[-2.3%, -1.2%]	8

Cycles

Results (secondary 1.7%)

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)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.1%)

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.0%	[0.0%, 0.0%]	4
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	4

Bootstrap: 763.545s -> 762.625s (-0.12%)
Artifact size: 325.56 MiB -> 325.60 MiB (0.01%)

rustbot assigned davidtwco Dec 26, 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 Dec 26, 2024

RalfJung mentioned this pull request Dec 26, 2024

Figure out which target features are required/incompatible for which ABI #131799

Open

15 tasks

This comment has been minimized.

Sign in to view

RalfJung force-pushed the abi-required-target-features branch from 79f8de2 to 849fb06 Compare December 26, 2024 18:52

This was referenced Dec 26, 2024

x86-32 "f16" ABI needs SSE, incompatible with i586 targets #131819

Open

aarch64-unknown-none-softfloat: ABI unsoundness when enabling "neon" feature #134375

Open

This comment has been minimized.

Sign in to view

RalfJung force-pushed the abi-required-target-features branch from 849fb06 to a25cfb0 Compare December 26, 2024 19:43

This comment has been minimized.

Sign in to view

RalfJung force-pushed the abi-required-target-features branch from a25cfb0 to e89c9ff Compare December 26, 2024 20:50

This comment has been minimized.

Sign in to view

RalfJung force-pushed the abi-required-target-features branch from e89c9ff to c221672 Compare December 26, 2024 21:34

This comment has been minimized.

Sign in to view

RalfJung force-pushed the abi-required-target-features branch from c221672 to b144ab7 Compare December 26, 2024 22:11

workingjubilee assigned workingjubilee and unassigned davidtwco Dec 26, 2024

taiki-e reviewed Dec 27, 2024

View reviewed changes

RalfJung force-pushed the abi-required-target-features branch from b144ab7 to 7563d57 Compare December 27, 2024 08:43

RalfJung commented Dec 27, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the abi-required-target-features branch from 958f3f3 to 8799588 Compare December 31, 2024 11:20

RalfJung added 6 commits December 31, 2024 12:41

explicitly model that certain ABIs require/forbid certain target feat…

2bf27e0

…ures

clean up target feature system; most of the toggleability is now hand…

cfae43d

…led by the ABI target feature check

adjust GCC backend

0a8cfc2

x86-64 hardfloat actually requires sse2

eb52742

add ABI target features *before* -Ctarget-features

912b729

arm: use target.llvm_floatabi over soft-float target feature

43ede97

RalfJung force-pushed the abi-required-target-features branch from 8799588 to 43ede97 Compare December 31, 2024 11:41

RalfJung removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 31, 2024

workingjubilee approved these changes Jan 5, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

add dedicated type for ABI target feature constraints

2e64b53

RalfJung force-pushed the abi-required-target-features branch from 887803d to 2e64b53 Compare January 5, 2025 09:46

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 6, 2025

bors merged commit feb32c6 into rust-lang:master Jan 6, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 6, 2025

This was referenced Jan 6, 2025

Add kl and widekl target features, and the feature gate #134814

Open

Partially stabilize LoongArch target features #135015

Open

RalfJung deleted the abi-required-target-features branch January 6, 2025 14:47

This was referenced Jan 7, 2025

The ABI of float types can be changed by -Ctarget-feature #116344

Open

Forbid disabling SSE on x86 targets that have SSE in their "baseline" #133611

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RalfJung commented Dec 26, 2024 •

edited

Loading

rustbot commented Dec 26, 2024

rustbot commented Dec 26, 2024

rustbot commented Dec 26, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

workingjubilee commented Dec 26, 2024

taiki-e Dec 27, 2024

RalfJung Dec 27, 2024

RalfJung commented Dec 27, 2024

RalfJung Dec 27, 2024

RalfJung Dec 27, 2024

RalfJung commented Dec 27, 2024

This comment has been minimized.

tgross35 commented Dec 27, 2024

bors commented Dec 31, 2024

RalfJung commented Dec 31, 2024

workingjubilee left a comment

This comment has been minimized.

workingjubilee commented Jan 5, 2025

bors commented Jan 5, 2025

bors commented Jan 5, 2025

bors commented Jan 6, 2025

rust-timer commented Jan 6, 2025

-                    "ilp32" | "lp64" => NOTHING,
+                    "ilp32" | "lp64" => {
+                        // Incompatible with e.
+                        (&[], &["e"])
+                    }

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

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

Conversation

RalfJung commented Dec 26, 2024 • edited Loading

rustbot commented Dec 26, 2024

rustbot commented Dec 26, 2024

rustbot commented Dec 26, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

workingjubilee commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Dec 27, 2024

This comment has been minimized.

tgross35 commented Dec 27, 2024

bors commented Dec 31, 2024

RalfJung commented Dec 31, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

This comment has been minimized.

workingjubilee commented Jan 5, 2025

bors commented Jan 5, 2025

bors commented Jan 5, 2025

bors commented Jan 6, 2025

rust-timer commented Jan 6, 2025

Overall result: ❌ regressions - no action needed

RalfJung commented Dec 26, 2024 •

edited

Loading