Rename `rustc_abi::Abi` to `BackendRepr` #132246

workingjubilee · 2024-10-28T03:24:08Z

Remove the confabulation of rustc_abi::Abi with what "ABI" actually means by renaming it to BackendRepr, and rename Abi::Aggregate to BackendRepr::Memory. The type never actually represented how things are passed, as that has to have PassMode considered, at minimum, but rather it just is how we represented some things to the backend. This conflation arose because LLVM, the primary backend at the time, would lower certain IR forms using certain ABIs. Even that only somewhat was true, as it broke down when one ventured significantly afield of what is described by the System V AMD64 ABI either by using different architectures, ABI-modifying IR annotations, the same architecture with different ISA extensions enabled, or other... unexpected delights.

Unfortunately both names are still somewhat of a misnomer right now, as people have written code for years based on this misunderstanding. Still, their original names are even moreso, and for better or worse, this backend code hasn't received as much maintenance as the rest of the compiler, lately. Actually arriving at a correct end-state will simply require us to disentangle a lot of code in order to fix, much of it pointlessly repeated in several places. Thus this is not an "actual fix", just a way to deflect further misunderstandings.

Acts on long-ago remarks from @eddyb as narrated by @RalfJung in Add infrastructure to "compute the ABI of a Rust type, described as a C type" compiler-team#672 (comment)
Part of Tracking issue for refactoring the way we represent function call ABIs #119183

r? @jieyouxu

rustbot · 2024-10-28T03:24:19Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

compiler-errors · 2024-10-28T03:54:19Z

Hate to bikeshed but why is this called "Ir"? We got lots of IRs in the compiler, and the impression I get is that type name is overly vague. It's like naming a type "Kind" with no qualifiers 😃

LayoutForm? LayoutKind? idk

compiler-errors

At the same time, reduce the need of some backends of rustc to snuffle around the internals of the Layout types by exposing another accessor, LayoutS::is_uninhabited.

Seems like there's quite a few callsites that are still doing layout.ir_form.is_uninhabited() rather than going thru the new accessor. I'd also be happy to quickly review that set of changes pulled out of this PR in particular, since it seems somewhat orthogonal to this rename.

Similarly, there are some callsites that call layout.ir_form.other_method() for other methods like is_sized() that could probably just be turned into direct calls on the layout type?

compiler/rustc_abi/src/layout.rs

workingjubilee · 2024-10-28T04:00:33Z

Because that is all it was ever supposed to be.

It does not actually meaningfully determine layout: there are many scalars that might be represented identically to aggregates and vice versa.
Likewise, it does not actually determine the calling convention, as there are many IR annotations that might be added via other routes, some of which have much more of an impact.

It primarily was intended to distinguish between "handle as an SSA value (scalar or vector), literally written using the syntactic forms that LLVMIR gives to SSA values, or the IR repr, or interact with it as a pointer to memory".

jieyouxu · 2024-10-28T04:02:22Z

I think I like LayoutForm more than IrForm, for the same reasoning -- we already have several IRs in the compiler. Or maybe even BackendLayout or BackendForm?

compiler-errors · 2024-10-28T04:02:43Z

Because that is all it was ever supposed to be.

Agreed that it's misleading, but I still think it needs a name that clearly denotes that it is a component of computations that are primarily concerning codegen and layout. Right now that name does not do that at all. "IR" means something completely different to the bulk of the compiler than what you seem to make it mean here.

Whether that's LayoutForm or something different, I'm really just asking for something more distinguishing.

workingjubilee · 2024-10-28T04:03:56Z

Happy to crank the name generator in my head, just wanted to clarify that going to "LayoutSomething" will add to the confusion.

workingjubilee · 2024-10-28T04:04:08Z

SsaForm or whatever?

jieyouxu · 2024-10-28T04:06:25Z

That sounds reasonable to me.

jieyouxu · 2024-10-28T04:07:18Z

r? @compiler-errors since you're already reviewing it

workingjubilee · 2024-10-28T04:09:09Z

Yeah I got the idea of "wait... accessors exist" only midway through this.

compiler-errors · 2024-10-28T04:11:21Z

I'm happy with SsaForm or something slightly more descriptive in that direction, but maybe let's wait a few hours (i.e. im gonna go sleep now) to see if anyone else on the backend side has stronger opinions than me.

RalfJung · 2024-10-28T05:57:35Z

SsaForm makes little sense to me since a codegen backend doesn't have to use SSA -- for instance, Miri uses this to inform the way it represents local variables, but there's no SSA involved.

In the MCP I suggested names like StorageClass or StorageKind. We could also use StorageForm. To me, "storage" makes sense since it is really a detail about how the rest of the compiler is supposed to store values of this type -- though it is always correct to ignore that and store it in-memory anyway.

workingjubilee · 2024-10-28T05:59:56Z

"Storage" just sounds like a synonym for "Layout" though.

workingjubilee · 2024-10-28T06:02:26Z

The unfortunate problem here is that this type currently does not answer a question, but we can't simply discard it without significantly more work.

To call it "Storage" implies it would answer the question of "how will this value be stored?"

But it does not.

RalfJung · 2024-10-28T06:02:56Z

Not at all? "layout" for us has the fairly clear meaning of being about field offsets, and related properties. It's like the layout of a page or website. "storage" has a completely different meaning than that? I don't follow at all TBH.

RalfJung · 2024-10-28T06:03:54Z

The type is a hint, answering the question of how values of this type shall be stored by backends if there are no other constraints that force it to be stored in-memory.

workingjubilee · 2024-10-28T06:12:37Z

The type is a hint, answering the question of how values of this type shall be stored by backends if there are no other constraints that force it to be stored in-memory.

PoliteAbiSuggestionIfItIsntTooMuchTrouble, then???

…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#129394 (Don't lint `irrefutable_let_patterns` on leading patterns if `else if` let-chains) - rust-lang#131856 (TypingMode: merge intercrate, reveal, and defining_opaque_types) - rust-lang#132246 (Rename `rustc_abi::Abi` to `BackendRepr`) - rust-lang#132322 (powerpc64-ibm-aix: update maintainters) - rust-lang#132327 (Point to Fuchsia team in platform support docs) - rust-lang#132332 (Use `token_descr` more in error messages) - rust-lang#132338 (Remove `Engine`) - rust-lang#132340 (cg_llvm: Consistently use safe wrapper function `set_section`) r? `@ghost` `@rustbot` modify labels: rollup

workingjubilee · 2024-10-30T05:40:21Z

@bors r-

tests/ui/abi/c-zst.other-linux.stderr

workingjubilee · 2024-10-30T07:06:27Z

Rewrote the platform-specific ZST ABI tests to now do cross-compilation, as they each only really need a single Sized lang-item. They are now also blessed.

RalfJung · 2024-10-30T07:13:09Z

tests/ui/abi/c-zst.rs

 //@ normalize-stderr-test: "(abi|pref|unadjusted_abi_align): Align\([1-8] bytes\)" -> "$1: $$SOME_ALIGN"
+
+// Ignore the ZST revisions


I don't understand the comment. What are we supposed to ignore?

The old code had two sections,"ignore the ZST" and "pass the ZST indirectly". That structure seems to have been lost and now it's just confusing.

I found the previous structure confusing too, tbh.

@RalfJung I don't feel confident that our handling of passing the ZST by indirection is correct for the ABIs in question.

huh, weird, it is.

I found the previous structure confusing too, tbh.

There are two kinds of targets: those where ZST are ignored, and those where they are passed indirectly (which may seem nonsensical but it's what it is). It is helpful to group the revisions for the test by which category a target falls into.

@RalfJung I did a bit more studying to satisfy my curiosity a bit more as to exactly why so many ABIs decide ZSTs consume space, and organized the revisions accordingly again, plus added some remarks as to the history. This hopefully will motivate the organization more for the next reader.

Thanks!

I was going to suggest a better place for the comment would be the code in rustc that implements this ABI logic. That used to be one central place but b1493ba recently moved it to the corresponding targets... which is probably a good call but makes it harder to to put a good comment in a central place.

Yeah, "the code may or may not be in one place" is basically the reason I've been leaving these sorts of comments by the test cases. Tests in many ways describe the spec that we're adhering to, but the implementation can be whatever... including "spread out across 100 modules".

I may find a better home for some of this text at some point, but not today.

This allows them to be blessed, regardless of platform.

workingjubilee · 2024-10-30T09:03:45Z

I think we're good now?
@bors r=compiler-errors

bors · 2024-10-30T09:03:48Z

📌 Commit 083a362 has been approved by compiler-errors

It is now in the queue for this repository.

…kingjubilee Rollup of 5 pull requests Successful merges: - rust-lang#129383 (Remap impl-trait lifetimes on HIR instead of AST lowering) - rust-lang#132210 (rustdoc: make doctest span tweak a 2024 edition change) - rust-lang#132246 (Rename `rustc_abi::Abi` to `BackendRepr`) - rust-lang#132267 (force-recompile library changes on download-rustc="if-unchanged") - rust-lang#132344 (Merge `HostPolarity` and `BoundConstness`) Failed merges: - rust-lang#132347 (Remove `ValueAnalysis` and `ValueAnalysisWrapper`.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#132246 - workingjubilee:campaign-on-irform, r=compiler-errors Rename `rustc_abi::Abi` to `BackendRepr` Remove the confabulation of `rustc_abi::Abi` with what "ABI" actually means by renaming it to `BackendRepr`, and rename `Abi::Aggregate` to `BackendRepr::Memory`. The type never actually represented how things are passed, as that has to have `PassMode` considered, at minimum, but rather it just is how we represented some things to the backend. This conflation arose because LLVM, the primary backend at the time, would lower certain IR forms using certain ABIs. Even that only somewhat was true, as it broke down when one ventured significantly afield of what is described by the System V AMD64 ABI either by using different architectures, ABI-modifying IR annotations, the same architecture **with different ISA extensions enabled**, or other... unexpected delights. Unfortunately both names are still somewhat of a misnomer right now, as people have written code for years based on this misunderstanding. Still, their original names are even moreso, and for better or worse, this backend code hasn't received as much maintenance as the rest of the compiler, lately. Actually arriving at a correct end-state will simply require us to disentangle a lot of code in order to fix, much of it pointlessly repeated in several places. Thus this is not an "actual fix", just a way to deflect further misunderstandings.

…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

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

Changes required due to rust-lang/rust#132246 (Rename `rustc_abi::Abi` to `BackendRepr`). Resolves: model-checking#3669

Changes required due to rust-lang/rust#132246 (Rename `rustc_abi::Abi` to `BackendRepr`). Resolves: #3669 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

workingjubilee · 2024-11-03T21:36:53Z

post-hoc note: If this commit broke your custom driver, codegen backend, or other compiler extension, try to move touching on the layout.backend_repr to the convenience functions now available, like LayoutData::is_uninhabited.

rustbot assigned jieyouxu Oct 28, 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 28, 2024

workingjubilee force-pushed the campaign-on-irform branch from f2cbe6e to ef67904 Compare October 28, 2024 03:25

compiler-errors reviewed Oct 28, 2024

View reviewed changes

compiler/rustc_abi/src/layout.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot assigned compiler-errors and unassigned jieyouxu Oct 28, 2024

This comment has been minimized.

Sign in to view

workingjubilee force-pushed the campaign-on-irform branch from bd156d7 to 23a6175 Compare October 28, 2024 05:24

This comment has been minimized.

Sign in to view

workingjubilee force-pushed the campaign-on-irform branch from 23a6175 to 9b44083 Compare October 28, 2024 05:42

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 30, 2024

workingjubilee commented Oct 30, 2024

View reviewed changes

tests/ui/abi/c-zst.other-linux.stderr Outdated Show resolved Hide resolved

workingjubilee force-pushed the campaign-on-irform branch from 3c145da to 37d3a92 Compare October 30, 2024 07:04

RalfJung reviewed Oct 30, 2024

View reviewed changes

workingjubilee force-pushed the campaign-on-irform branch from 37d3a92 to e25916c Compare October 30, 2024 08:26

workingjubilee added 2 commits October 30, 2024 01:41

tests: cross-compile multi-platform ZST ABI tests

0b9d1eb

This allows them to be blessed, regardless of platform.

tests: Bless rustc_abi::Abi::Aggregate => ::Memory

083a362

workingjubilee force-pushed the campaign-on-irform branch from e25916c to 083a362 Compare October 30, 2024 08:43

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2024

workingjubilee mentioned this pull request Oct 30, 2024

Rollup of 5 pull requests #132371

Merged

bors merged commit 847b6fe into rust-lang:master Oct 31, 2024
6 checks passed

rustbot added this to the 1.84.0 milestone Oct 31, 2024

workingjubilee deleted the campaign-on-irform branch October 31, 2024 01:27

workingjubilee mentioned this pull request Oct 31, 2024

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

Open

19 tasks

tautschnig added a commit to tautschnig/kani that referenced this pull request Nov 1, 2024

Upgrade toolchain to 2024-11-01

6d9fc12

Changes required due to rust-lang/rust#132246 (Rename `rustc_abi::Abi` to `BackendRepr`). Resolves: model-checking#3669

tautschnig mentioned this pull request Nov 1, 2024

Upgrade toolchain to 2024-11-01 model-checking/kani#3671

Merged

		//@ normalize-stderr-test: "(abi\|pref\|unadjusted_abi_align): Align\([1-8] bytes\)" -> "$1: $$SOME_ALIGN"

		// Ignore the ZST revisions

Rename rustc_abi::Abi to BackendRepr #132246

Rename rustc_abi::Abi to BackendRepr #132246

Conversation

workingjubilee commented Oct 28, 2024 • edited Loading

rustbot commented Oct 28, 2024

compiler-errors commented Oct 28, 2024

compiler-errors left a comment • edited Loading

Choose a reason for hiding this comment

workingjubilee commented Oct 28, 2024

jieyouxu commented Oct 28, 2024 • edited Loading

compiler-errors commented Oct 28, 2024 • edited Loading

This comment has been minimized.

workingjubilee commented Oct 28, 2024

workingjubilee commented Oct 28, 2024 • edited Loading

jieyouxu commented Oct 28, 2024

jieyouxu commented Oct 28, 2024

workingjubilee commented Oct 28, 2024

compiler-errors commented Oct 28, 2024

This comment has been minimized.

This comment has been minimized.

RalfJung commented Oct 28, 2024

workingjubilee commented Oct 28, 2024

workingjubilee commented Oct 28, 2024

RalfJung commented Oct 28, 2024

RalfJung commented Oct 28, 2024

workingjubilee commented Oct 28, 2024

workingjubilee commented Oct 30, 2024

workingjubilee commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Oct 30, 2024

bors commented Oct 30, 2024

workingjubilee commented Nov 3, 2024

Rename `rustc_abi::Abi` to `BackendRepr` #132246

Rename `rustc_abi::Abi` to `BackendRepr` #132246

workingjubilee commented Oct 28, 2024 •

edited

Loading

compiler-errors left a comment •

edited

Loading

jieyouxu commented Oct 28, 2024 •

edited

Loading

compiler-errors commented Oct 28, 2024 •

edited

Loading

workingjubilee commented Oct 28, 2024 •

edited

Loading

workingjubilee Oct 30, 2024 •

edited

Loading