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". 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.

This is best reviewed commit-by-commit. The first commit adds the new abi_required_features function. For now, that function supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;) Both -Ctarget-feature and #[target_feature] are updated to ensure we follow the rules of the ABI. We also 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.

As a side-effect this also (unstably) allows enabling x87 when that is harmless.

This should also prepare us for requiring SSE on x86-32bit when we want to use that for our ABI (and for float semantics sanity), see #133611.

The last commit marks SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2.

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.

workingjubilee · 2024-12-27T23:59:14Z

@tgross35 We already have aarch64-unknown-none-softfloat.

@ojeda Why does the kernel not use it?

RalfJung · 2024-12-28T07:49:20Z

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.

Thus far, the lang and libs-api teams have insisted that all targets must support float types. They may be software emulated but they cannot be missing. In principle this is entirely feasible, it is just not currently supported by LLVM.

…ures

…led by the ABI target feature check

rustbot · 2024-12-28T08:07:10Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

RalfJung · 2024-12-28T08:10:57Z

compiler/rustc_target/src/target_features.rs

+                // It's unclear how the `abi` and the `soft-float` target feature interact.
+                // We use the `abi` as our primary signal, and force `soft-float` to match.
+                match &*self.abi {
+                    "eabi" => (&["soft-float"], &[]),


Turns out the arm-linux-androideabi target sets the eabi ABI, but does not set +soft-float. What does that even mean? Is it a valid combination?

With my PR, +soft-float now gets added automatically, which is why the run-make test needed adjustments.

@maurer @chriswailes @mgeisler almost all our ARM eabi targets set +soft-float; the android targets seems to be the only exception (at least they are the only exception I have found so far). Is there a reason for this or is it just an oversight?

Turns out the arm-linux-androideabi target sets the eabi ABI, but does not set +soft-float. What does that even mean? Is it a valid combination?

Yes, it is a valid combination. It means, in essence, "pass floats via integer registers, but internal to functions, use the hardware float instructions".

It makes sense when your original baseline for target support for an architecture was originally "the hardware FPU is... optional? or... the exact one you're using isn't specified, at least...?" and you don't want to break ABI but have nonetheless raised the hardware requirements to run your target.

Yes, it is a valid combination. It means, in essence, "pass floats via integer registers, but internal to functions, use the hardware float instructions".

AFAIK, the abi field is not forwarded to LLVM in any form. (We have llvm_abiname for that.) So I can't align what you are saying with what the Rust compiler does.

...Oh, my apologies, I was only answering the abstract question because you said ABI and not abi. As I mentioned, the abi field is just a string intended to identify what ABI we are supposed to be using. Despite it being a valid notion, we may not actually be implementing anything correctly here, as I have no idea what set of mystical incantations LLVM expects here (as each target backend seems to implement the idea of "a soft-float ABI may exist" slightly differently).

My understanding was that +soft-float,+fpregs is how we request a softfloat ABI but using float instructions where possible. But I am not sure whether that actually works.

ARM targets have -float-abi in clang (which we expose via -Csoft-float), which may be a more reliable way to control this than target features? But that makes me wonder what the point is of having both that flag and the soft-float target feature...

ojeda · 2024-12-28T14:13:45Z

@tgross35 We already have aarch64-unknown-none-softfloat.

@ojeda Why does the kernel not use it?

Cc @JamieCunliffe -- it may be possible to use it (from a quick test, it seems to work).

workingjubilee · 2024-12-28T22:18:45Z

Cool. Just Doing That would be appreciated. I'd rather special-case one target tuple for a while for your benefit, if necessary, while we hash out some details, than have that particular need affect how we handle other targets.

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 added 5 commits December 28, 2024 09:01

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

189811e

…ures

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

c7818e3

…led by the ABI target feature check

adjust GCC backend

a0034b0

x86-64 hardfloat actually requires sse2

f18cd39

add ABI target features *before* -Ctarget-features

5c6ed53

RalfJung force-pushed the abi-required-target-features branch from e42ad00 to 17b820c Compare December 28, 2024 08:07

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Dec 28, 2024

RalfJung commented Dec 28, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the abi-required-target-features branch from 17b820c to 6861049 Compare December 28, 2024 09:26

This comment has been minimized.

Sign in to view

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

958f3f3

RalfJung force-pushed the abi-required-target-features branch from 6861049 to 958f3f3 Compare December 28, 2024 10:03

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

workingjubilee commented Dec 27, 2024

RalfJung commented Dec 28, 2024 via email

rustbot commented Dec 28, 2024

RalfJung Dec 28, 2024 •

edited

Loading

RalfJung Dec 28, 2024

workingjubilee Dec 28, 2024 •

edited

Loading

RalfJung Dec 28, 2024

workingjubilee Dec 28, 2024 •

edited

Loading

RalfJung Dec 29, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

ojeda commented Dec 28, 2024

workingjubilee commented Dec 28, 2024 •

edited

Loading

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

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

Are you sure you want to change the base?

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

taiki-e Dec 27, 2024

Choose a reason for hiding this comment

RalfJung Dec 27, 2024

Choose a reason for hiding this comment

RalfJung commented Dec 27, 2024

RalfJung Dec 27, 2024

Choose a reason for hiding this comment

RalfJung Dec 27, 2024

Choose a reason for hiding this comment

RalfJung commented Dec 27, 2024

This comment has been minimized.

tgross35 commented Dec 27, 2024

workingjubilee commented Dec 27, 2024

RalfJung commented Dec 28, 2024 via email

rustbot commented Dec 28, 2024

RalfJung Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Dec 28, 2024

Choose a reason for hiding this comment

workingjubilee Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Dec 28, 2024

Choose a reason for hiding this comment

workingjubilee Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

ojeda commented Dec 28, 2024

workingjubilee commented Dec 28, 2024 • edited Loading

RalfJung commented Dec 26, 2024 •

edited

Loading

RalfJung Dec 28, 2024 •

edited

Loading

workingjubilee Dec 28, 2024 •

edited

Loading

workingjubilee Dec 28, 2024 •

edited

Loading

RalfJung Dec 29, 2024 •

edited

Loading

workingjubilee commented Dec 28, 2024 •

edited

Loading