Use `FieldIdx` in `FieldsShape` #109915

scottmcm · 2023-04-03T22:24:22Z

Finally got to the main motivating example from rust-lang/compiler-team#606 :)

rustbot · 2023-04-03T22:24:28Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustbot · 2023-04-03T22:24:28Z

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

scottmcm · 2023-04-03T22:27:36Z

compiler/rustc_ty_utils/src/layout.rs

@@ -492,8 +481,7 @@ fn layout_of_uncached<'tcx>(
 enum SavedLocalEligibility {
    Unassigned,
    Assigned(VariantIdx),
-    // FIXME: Use newtype_index so we aren't wasting bytes
-    Ineligible(Option<u32>),
+    Ineligible(Option<FieldIdx>),


Hooray for fixing FIXMEs from 2019 🙂

scottmcm · 2023-04-03T22:29:42Z

compiler/rustc_index/src/vec.rs

+    /// and `inverse_memory_index` (memory order to source field order).
+    /// See also `FieldsShape::Arbitrary::memory_index` for more details.
+    // FIXME(eddyb) build a better abstraction for permutations, if possible.
+    pub fn invert_bijective_mapping(&self) -> IndexVec<J, I> {


This code was in three places (the two obvious ones and https://github.com/rust-lang/rust/pull/109915/files#diff-bd58d97c1958029df344a30d473950e0195516d1153cbcc6314ff01afb87d12aR1191), so I figured promoting it to here was reasonable enough.

compiler/rustc_abi/src/layout.rs

oli-obk · 2023-04-04T09:13:18Z

compiler/rustc_abi/src/layout.rs

+        let memory_index = if optimize {
+            inverse_memory_index.invert_bijective_mapping()
+        } else {
+            debug_assert!(inverse_memory_index.iter().copied().eq(fields.indices()));
+            IndexVec::from_raw(transmute_index_vec(inverse_memory_index.raw))
+        };


I think the right optimization here may be to turn the memory_index field into an Option and if it's none, just use the index you would have used to index into it instead of actually having a vec![0, 1, 2, 3, ...] for all the common use cases.

It should also not be terribly hot, so yea, please use a safe version of this and we can look into optimizing it as a follow-up

compiler/rustc_ty_utils/src/layout.rs

Finally got to the main motivating example from the MCP :)

oli-obk · 2023-04-05T07:50:11Z

@bors try @rust-timer queue

bors · 2023-04-05T07:50:22Z

⌛ Trying commit 21bb8ef with merge 6aaacfc5ec967a8d7adb686522f6e9f06d0f07f6...

bors · 2023-04-05T09:34:03Z

☀️ Try build successful - checks-actions
Build commit: 6aaacfc5ec967a8d7adb686522f6e9f06d0f07f6 (6aaacfc5ec967a8d7adb686522f6e9f06d0f07f6)

bors · 2023-04-05T09:34:04Z

☀️ Try build successful - checks-actions
Build commit: 6aaacfc5ec967a8d7adb686522f6e9f06d0f07f6 (6aaacfc5ec967a8d7adb686522f6e9f06d0f07f6)

rust-timer · 2023-04-05T11:44:41Z

Finished benchmarking commit (6aaacfc5ec967a8d7adb686522f6e9f06d0f07f6): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[1.4%, 2.9%]	3
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	-3.2%	[-3.2%, -3.2%]	1

Cycles

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

oli-obk · 2023-04-05T13:02:55Z

@bors r+

bors · 2023-04-05T13:04:31Z

📌 Commit 21bb8ef has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-04-06T07:39:02Z

⌛ Testing commit 21bb8ef with merge 2824db3...

bors · 2023-04-06T09:54:51Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 2824db3 to master...

rust-timer · 2023-04-06T11:10:25Z

Finished benchmarking commit (2824db3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[3.3%, 4.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.0%	[2.0%, 5.2%]	13
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-4.0%	[-4.4%, -3.4%]	3
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

scottmcm marked this pull request as ready for review April 3, 2023 22:24

rustbot assigned oli-obk Apr 3, 2023

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 Apr 3, 2023

scottmcm commented Apr 3, 2023

View reviewed changes

erikdesjardins reviewed Apr 4, 2023

View reviewed changes

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

oli-obk reviewed Apr 4, 2023

View reviewed changes

compiler/rustc_ty_utils/src/layout.rs Show resolved Hide resolved

Use FieldIdx in FieldsShape

21bb8ef

Finally got to the main motivating example from the MCP :)

scottmcm force-pushed the layout-indexvec branch from e41b45d to 21bb8ef Compare April 4, 2023 19:38

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2023

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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2023

bors merged commit 2824db3 into rust-lang:master Apr 6, 2023

rustbot added this to the 1.70.0 milestone Apr 6, 2023

bors mentioned this pull request Apr 6, 2023

Improve niche placement by trying two strategies and picking the better result #108106

Merged

scottmcm deleted the layout-indexvec branch April 6, 2023 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `FieldIdx` in `FieldsShape` #109915

Use `FieldIdx` in `FieldsShape` #109915

scottmcm commented Apr 3, 2023

rustbot commented Apr 3, 2023

rustbot commented Apr 3, 2023

scottmcm Apr 3, 2023

scottmcm Apr 3, 2023

oli-obk Apr 4, 2023

oli-obk commented Apr 5, 2023

This comment has been minimized.

bors commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 5, 2023

This comment has been minimized.

rust-timer commented Apr 5, 2023

oli-obk commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

rust-timer commented Apr 6, 2023

Use FieldIdx in FieldsShape #109915

Use FieldIdx in FieldsShape #109915

Conversation

scottmcm commented Apr 3, 2023

rustbot commented Apr 3, 2023

rustbot commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Apr 5, 2023

This comment has been minimized.

bors commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 5, 2023

This comment has been minimized.

rust-timer commented Apr 5, 2023

Overall result: ✅ improvements - no action needed

oli-obk commented Apr 5, 2023

bors commented Apr 5, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

rust-timer commented Apr 6, 2023

Overall result: no relevant changes - no action needed

Use `FieldIdx` in `FieldsShape` #109915

Use `FieldIdx` in `FieldsShape` #109915