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

Tracking issue for refactoring the way we represent function call ABIs #119183

Open
1 of 19 tasks
RalfJung opened this issue Dec 21, 2023 · 9 comments
Open
1 of 19 tasks

Tracking issue for refactoring the way we represent function call ABIs #119183

RalfJung opened this issue Dec 21, 2023 · 9 comments
Assignees
Labels
A-ABI Area: Concerning the application binary interface (ABI) C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Dec 21, 2023

This is the issue tracking implementation of rust-lang/compiler-team#672. Note that we do not have a final design yet; the best way to represent call ABI, and to disentangle it from the "storage kind" of a type (which is what the Abi type currently largely represents) is yet to be determined.

  • The core need is that a Rust type must have its ABI computed in a manner that is not reimplemented for every target and every codegen backend. e.g. the traversal over types should skip repr(transparent) in essentially every case. Then every target architecture must be ported to it:
    • aarch64
    • arm
    • csky
    • loongarch
    • loongarch64
    • mips
    • mips64
    • powerpc
    • powerpc64
    • riscv32
    • riscv64
    • sparc
    • sparc64
    • x86
    • x86_64
  • layout.abi is not about actual ABI, but only about IR codegen: handling the types as SSA values versus handling them as memory locations.

See in particular this comment.

Implementation history

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 21, 2023
@bjorn3 bjorn3 added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC A-ABI Area: Concerning the application binary interface (ABI) C-cleanup Category: PRs that clean code up or issues documenting cleanup. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Dec 21, 2023
@bjorn3

This comment has been minimized.

@RalfJung

This comment has been minimized.

@RalfJung
Copy link
Member Author

Looks like fixing #117480 may be blocked on this.

@fmease fmease added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 16, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Oct 16, 2024

I've started work on cleaning up ABI code. The first few PRs are just going to be moving stuff around so that we can actually co-locate as much of the ABI code as possible:

I'll try to implement the MCP proper as I go along.

@RalfJung
Copy link
Member Author

Awesome. :-)

If you want feedback on some sketches of what the ABI might look like, feel free to post them here.

@workingjubilee workingjubilee self-assigned this Oct 17, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 31, 2024
…bi, r=jieyouxu,compiler-errors

compiler: Move `rustc_target::spec::abi::Abi` to `rustc_abi::ExternAbi`

Lift `enum Abi` from its rather odd place in the middle of rustc_target, and make it available again from rustc_abi. You know, the crate where you would expect the enum that describes all the ABIs to be? The platform-neutral ones, at least. This will help further refactoring of how we handle ABIs in the near future[^0].

Rename `Abi` to `ExternAbi` because quite a lot of the compiler overloads the concept of "ABI" enough that the existing name is imprecise and it is often renamed _anyway_. Often this was to avoid conflicts with the *other* type formerly known as `Abi` (now named BackendRepr[^1]), but sometimes it is just for clarity, and this name seems more self-explanatory. It does get reexported, though, using its old name, to reduce the odds of merge-conflicting over the entire tree.

All of `ExternAbi`'s friends come along for the ride, which costs adding some optional dependencies to the rustc_abi crate. However, all of this also allows simply moving three crates entirely off rustc_target:
- rustc_hir_pretty
- rustc_lint_defs
- rustc_mir_build

This odd selection is mostly to demonstrate a secondary motivation: The majority of the front-end of the compiler should be as target-agnostic as possible, and it is easier to assure this if they simply don't depend on the crate that describes targets. Note that I didn't migrate crates that don't benefit from it in this way yet, and I didn't survey every last crate.

[^0]: This is being undertaken as part of rust-lang#119183
[^1]: rust-lang#132246
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 1, 2024
…bi, r=jieyouxu,compiler-errors

compiler: Move `rustc_target::spec::abi::Abi` to `rustc_abi::ExternAbi`

Lift `enum Abi` from its rather odd place in the middle of rustc_target, and make it available again from rustc_abi. You know, the crate where you would expect the enum that describes all the ABIs to be? The platform-neutral ones, at least. This will help further refactoring of how we handle ABIs in the near future[^0].

Rename `Abi` to `ExternAbi` because quite a lot of the compiler overloads the concept of "ABI" enough that the existing name is imprecise and it is often renamed _anyway_. Often this was to avoid conflicts with the *other* type formerly known as `Abi` (now named BackendRepr[^1]), but sometimes it is just for clarity, and this name seems more self-explanatory. It does get reexported, though, using its old name, to reduce the odds of merge-conflicting over the entire tree.

All of `ExternAbi`'s friends come along for the ride, which costs adding some optional dependencies to the rustc_abi crate. However, all of this also allows simply moving three crates entirely off rustc_target:
- rustc_hir_pretty
- rustc_lint_defs
- rustc_mir_build

This odd selection is mostly to demonstrate a secondary motivation: The majority of the front-end of the compiler should be as target-agnostic as possible, and it is easier to assure this if they simply don't depend on the crate that describes targets. Note that I didn't migrate crates that don't benefit from it in this way yet, and I didn't survey every last crate.

[^0]: This is being undertaken as part of rust-lang#119183
[^1]: rust-lang#132246
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 1, 2024
Rollup merge of rust-lang#132385 - workingjubilee:move-abi-to-rustc-abi, r=jieyouxu,compiler-errors

compiler: Move `rustc_target::spec::abi::Abi` to `rustc_abi::ExternAbi`

Lift `enum Abi` from its rather odd place in the middle of rustc_target, and make it available again from rustc_abi. You know, the crate where you would expect the enum that describes all the ABIs to be? The platform-neutral ones, at least. This will help further refactoring of how we handle ABIs in the near future[^0].

Rename `Abi` to `ExternAbi` because quite a lot of the compiler overloads the concept of "ABI" enough that the existing name is imprecise and it is often renamed _anyway_. Often this was to avoid conflicts with the *other* type formerly known as `Abi` (now named BackendRepr[^1]), but sometimes it is just for clarity, and this name seems more self-explanatory. It does get reexported, though, using its old name, to reduce the odds of merge-conflicting over the entire tree.

All of `ExternAbi`'s friends come along for the ride, which costs adding some optional dependencies to the rustc_abi crate. However, all of this also allows simply moving three crates entirely off rustc_target:
- rustc_hir_pretty
- rustc_lint_defs
- rustc_mir_build

This odd selection is mostly to demonstrate a secondary motivation: The majority of the front-end of the compiler should be as target-agnostic as possible, and it is easier to assure this if they simply don't depend on the crate that describes targets. Note that I didn't migrate crates that don't benefit from it in this way yet, and I didn't survey every last crate.

[^0]: This is being undertaken as part of rust-lang#119183
[^1]: rust-lang#132246
@RalfJung
Copy link
Member Author

Looks like there is also some interest on the LLVM side to improve their ABI handling. Would be nice if we could benefit from that -- though we also have other backends, so maybe there's no way we can avoid having our own implementation of the C ABI... OTOH, our current PassMode is very LLVM-specific, so I don't know how much other backends can even use it today.

@bjorn3
Copy link
Member

bjorn3 commented Nov 19, 2024

our current PassMode is very LLVM-specific, so I don't know how much other backends can even use it today.

cg_clif makes full use of it. In fact PassMode is at pretty much exactly the right level of abstraction as Cranelift needs (it doesn't support high level types and requires you to decompose everything into primitive values and for struct arguments a pointer argument with ArgumentPurpose::StructArg and for complex return types it requires you to pass a return area pointer. all of which PassMode makes pretty easy to do). The only problems I have with it are that LLVM silently accepts things that IMO shouldn't be accepted like PassMode::Direct for a return value that doesn't fit in registers. (PassMode::Indirect { on_stack: false } should be used for that instead.) We also now have at least some sanity checks for non-sensical things like PassMode::Direct for a complex struct.

@workingjubilee
Copy link
Member

Yeah, IMO our main ask from LLVM should be "more hard errors instead of silently making up some weird shit, please", with probably more IR parameter attributes to enable "please make up some weird shit on purpose".

@RalfJung
Copy link
Member Author

RalfJung commented Nov 22, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants