layout: Store inverse memory index in FieldsShape::Arbitrary

[moulins]

All usages of memory_index start by calling invert_bijective_mapping, so storing the inverted mapping directly saves some work and simplifies the code.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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

[workingjubilee]

huh. I wonder what it's inverted relative to?

[workingjubilee]

@moulins I have no objection to the substance of the change but "inverse" suggests a relativeness this doesn't capture... IMO this is still just a "memory index" until you do something like specify the other half of the pair, whether it's the key or value, at which point they might adopt a logical ordering.

mem_src_field_index maybe? idk.

[workingjubilee]

kinda curious if this has any effect on perf or if it's just logical cleanup

@bors try @rust-timer queue

[moulins]

I have no objection to the substance of the change but "inverse" suggests a relativeness this doesn't capture... IMO this is still just a "memory index" until you do something like specify the other half of the pair, whether it's the key or value, at which point they might adopt a logical ordering.

mem_src_field_index maybe? idk

Hm, good point... what about in_memory_order?

[workingjubilee]

something like that sounds good to me!

[rust-bors]

☀️ Try build successful (CI)
Build commit: ed52dc8 (ed52dc8f27781eef22c18d699984ef94c758cc92, parent: f794a08738958bbef01c3dcab34ec21d3da1e51a)

[rust-timer]

Finished benchmarking commit (ed52dc8): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.0%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 1.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.8%, 1.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[1.8%, 1.8%]	1

Cycles

Results (primary 2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 481.694s -> 485.432s (0.78%)
Artifact size: 390.59 MiB -> 390.53 MiB (-0.01%)

[RalfJung]

If iterating in memory order is now cheap, maybe we should just always do that... in fact that could unlock a bunch of follow-up cleanup here. Probably best to make all that a follow-up PR, but a good indication that this PR moves us in the right direction. :)

[moulins]

So you'd propose removing aggregate_field_iter entirely and and simply iterate the fields directly?

[RalfJung]

Yes. But I would suggest doing it in a separate PR so we can separately benchmark that change.

[moulins]

Renamed inverse_memory_index to in_memory_order, and updated some UI tests that I missed previously.

EDIT: I just noticed that rust-analyzer has its own version of FieldsShape, should I perform the same change here too?

[workingjubilee]

@moulins I don't think this type is related in a meaningful way? https://github.com/rust-lang/rust/blob/main/src/tools/rust-analyzer/crates/hir-def/src/item_tree.rs#L519

Do you mean something else?

[workingjubilee]

To be clear, I am 100% certain that rust-analyzer uses rustc_abi::FieldsShape also but have no idea what uses need to be changed if any.

[moulins]

You're totally right; from what I can see, the uses are here in Layout::{tail_padding, largest_padding}, but for some reason my ./x check still succeeds and trying to rename these uses causes an error.

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.