Skip to content
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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 26, 2024

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
Copy link
Collaborator

rustbot commented Dec 26, 2024

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
Copy link
Collaborator

rustbot commented Dec 26, 2024

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 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
@rustbot
Copy link
Collaborator

rustbot commented Dec 26, 2024

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

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 RalfJung force-pushed the abi-required-target-features branch from b144ab7 to 7563d57 Compare December 27, 2024 08:43
@RalfJung
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants