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 target_abi = "[ilp]{2,3}[3264]{2}[fdq]?" to all RV[3264]{2}I targets #830

Open
6 of 12 tasks
workingjubilee opened this issue Jan 26, 2025 · 6 comments
Open
6 of 12 tasks
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Jan 26, 2025

Proposal

We added target_abi specifically to cover differences between soft float and hard float ABIs in targets, and potentially other interesting ABI differences. Here, we would simply add the appropriate target_abi string to all the RISCV targets that we support, essentially directly copied from the RISCV ABI spec for how these should be written:

  • "ilp32" - an RV32I target, passing floats as integers or on the stack
  • "ilp32f" - an RV32I target, allowing f32 to be passed in registers
  • "ilp32d" - an RV32I target, allowing f64 to be passed in registers
  • "lp64" - an RV64I target, passing floats as integers or on the stack
  • "lp64f" - an RV64I target with F registers enabled, allowing passing f32 in registers
  • "lp64d" - an RV64I target with D registers enabled, allowing f64 to be passed in registers
  • "lp64q" - an RV64I target with Q registers enabled, allowing use of the 128-bit hard float ABI (f128 can be passed directly in registers)
  • "ilp32e" - an RV32E ("Embedded") target with only 16 integer registers available, thus lacking the RV32I ("Integer") instruction set. Note that this ABI can be executed on an RV32I CPU!
  • "ilp32ef" - an RV32E ("Embedded") target with only 16 integer registers available, thus lacking the RV32I ("Integer") instruction set, thus lacking the entire Integer instruction set, but with F registers enabled, allowing passing f32 in registers. Note that this ABI can be executed on an RV32IF CPU.

NOTE: Approval of this MCP does not constitute implicit approval of adding targets that use all of the above target_abi strings. Only those with a checkmark are present in the codebase, and only those would be approved for now. This can already be observed because the target_abi will match whatever the llvm_abiname would be for a RISCV target: https://github.com/search?q=repo%3Arust-lang%2Frust+llvm_abiname&type=code

This was already done for the riscv32e targets in rust-lang/rust#134358 as they were added on an experimental basis, and otherwise there is no meaningful way to write a cfg option that distinguished between the RV32E or RV32I targets. They would have to use cfg(target_feature) with an unstable feature, and due to how the "feature" works it might not be something that actually makes sense or is correct, and may not be something we want to expose as a target_feature in actuality.

There is only one drawback of doing this: if some OS target expects to be able to use something like target_abi = "macabi", then this conflicts with the RISCV usage. There are numerous reasons this is not a substantial drawback:

  • Despite having the letters 'a', 'b', and 'i' in the string, placed in direct succession of each other, target_abi = "macabi" is more about the software environment ("build iOS apps slightly differently so they run on a host that has macOS libraries"), which is usually described via target_env. As some other mechanism should probably have been used to express target_abi = "macabi", we should discourage such uses anyways, as they invite confusion.
  • target_abi = "macabi" has very little to do with the calling convention or stack layout, which other uses of target_abi, predating the RV32E targets, do describe, such as target_abi = "eabihf" or target_abi = "eabi" for all the 32-bit Arm targets. In other words, this drawback already existed when target_abi = "macabi" was added, and using the same slot for conflicting meanings was not discussed to any extent that I am aware of.
  • Any such peculiar and opinionated uses of target_abi almost invariably do imply an actual ABI, because they are used for a very particular target variance, so the issue described above is a philosophical but not practical concern: there is no expected case where a target_abi would imply two different RISCV ABIs, even if the Darwin kernel (or NT kernel, for that matter) is ported to RISCV.

As a reminder, please take discussion to the Zulip thread.

This is an MCP because: it adds to the definition of a bunch of tier 2 targets in a strongly precedent-setting but additive way. It seems consistent with using the target_abi string as, er, the string that describes the ABI used by software for the target. Room for uncertainty is mostly because RFC 2992 is somewhat sparse on explaining "so what is an actual ABI difference that should require us to pick a specific target_abi string?" I present this opinion for how this should be done for the RISCV targets based on my research and expertise.

Mentors or Reviewers

As a reminder, please take discussion to the Zulip thread.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@workingjubilee workingjubilee added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Jan 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 26, 2025

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rfcbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rfcbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jan 26, 2025
@romancardenas
Copy link

LGTM. We rely on a riscv-target-parser crate to enumerate active RISC-V ISA extensions and act accordingly. Using target_abi can simplify our workflow.

I am far from an expert on the Rust compiler workflow, but will try to catch up and contribute here.

@rmsyn
Copy link

rmsyn commented Jan 29, 2025

From the latest specification, it looks like there is/will be ilp64e{fdq} ABI.

The restriction for not combining e with d, f, and q also looks like it was lifted (which you already added with ilp32ef).

@Noratrieb
Copy link
Member

This issue is not meant to be used for technical discussion. There is a Zulip stream for that.

See the rustbot message above.

@rmsyn
Copy link

rmsyn commented Jan 30, 2025

This issue is not meant to be used for technical discussion. There is a Zulip stream for that.

See the rustbot message above.

Ah, thanks @Noratrieb, maybe you could ask @workingjubilee to open the Zulip thread?

As a reminder, please take discussion to the Zulip thread.

Notice how all of the Zulip links simply link back to this issue?

@saethlin
Copy link
Member

saethlin commented Jan 30, 2025

I've fixed the issue description, the link goes the right place now.

I'm sure Jubilee just pasted the wrong link into the issue description. The link in the comment by rustbot has always been correct.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

7 participants