Mir-Opt for copying enums with large discrepancies by JulianKnodt · Pull Request #85158 · rust-lang/rust

JulianKnodt · 2021-05-10T20:40:26Z

I have been meaning to make this for quite a while, based off of this hackmd.

I'm not sure where to put this opt now that I've made it, so I'd appreciate suggestions on that!
It's also one long chain of statements, not sure if there's a more friendly format to make it.

r? @tmiasko
I would r oli but he's on leave so he suggested I r tmiasko or wesleywiser.

tmiasko

Hmm, so the idea is to transform: _a = _b; into:

_index = discriminant(_b);
_bytes = [size of each variant in bytes][_index]
CopyNonOverlapping(src: &_b as *const u8, dst: &_a as *mut u8, count: _bytes);

The transformation is localized to a single assignment, requires layout information, some aspects of it feel non-trivial to express directly in MIR (e.g., preserving the alignment). Maybe codegen would be a better fit?

tmiasko · 2021-05-12T15:46:39Z

This general pattern for copying the data appears to be opaque to memcpy optimizations, i.e., every single transformed copy remains. Any ideas how this could be improved?

scottmcm · 2021-05-12T16:31:32Z

Maybe codegen would be a better fit?

That would mean it would have the monomorphized versions of the enum too, right? Could be handy for anything using a generic Result and such...

JulianKnodt · 2021-05-12T18:04:35Z

Mmmm I've not written anything for codegen at all, but if that makes more sense I can close this and move it to that, altho seeing a guide before it would be helpful.

tmiasko · 2021-05-12T19:15:48Z

As you prefer. The current implementation is not that far from the point where we could run some tests on it. The one missing component is a mapping from a discriminant to a variant index (or limiting the transformation to the cases where there is direct correspondence between the two).

JulianKnodt · 2021-05-12T22:39:46Z

ah I thought reading from the array of sizes was handled around line 132, but I guess mapping discriminants to variant idxes is not guaranteed to be 1-1, is there anyway to check that?

bjorn3 · 2021-05-13T07:54:12Z

but I guess mapping discriminants to variant idxes is not guaranteed to be 1-1, is there anyway to check that?

I believe

enumFoo{Bar = 2,Baz = 0,}

has variant index 0 for Bar, but discriminant 2 and variant index 1 for Baz, but discriminant 0.

tmiasko · 2021-05-13T09:49:30Z

mapping discriminants to variant idxes is not guaranteed to be 1-1, is there anyway to check that?

The InterpCx::read_discriminant exercises related API and contains explanatory comments, might be good starting point.

camelid · 2021-05-28T00:41:00Z

Does this fix #54360?

JulianKnodt · 2021-05-28T02:12:50Z

I believe so? I don't remember the original issue.

tmiasko · 2021-05-28T16:27:50Z

The max - min criterion is based on the most optimistic scenario.
Consider two enums with the same maximum absolute deviation in variant sizes. The first enum has one large outlier and remaining variants are small, the second enum conversely has one small outlier and remaining variants are large. Both enums would be be equally good candidates under current criterion, but the first one would seem like a much better candidate.
What about assuming that variants are uniformly distributed and calculating expected reduction in the number of bytes copied?

oops I never directly addressed this, I'm not sure if uniform distribution makes a ton of sense, as I'd expect if this is hit that one variant is probably significantly larger than the others. Probably something to experiment with.

tmiasko · 2021-05-28T16:33:53Z

In terms of placement in MIR pipeline, I would put it towards the end, somewhere after SimplifyLocals, maybe just last? I wouldn't expect it to create any new optimization opportunities.

oli-obk · 2023-02-04T11:49:48Z

@bors ping

oli-obk · 2023-02-04T11:51:32Z

@bors r=wesleywiser

bors · 2023-02-04T11:51:34Z

📌 Commit d87d939f99fcb16b3960cd7238405b4b5e0bc391 has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2023-02-04T15:00:29Z

⌛ Testing commit d87d939f99fcb16b3960cd7238405b4b5e0bc391 with merge 97a7629360c6bd2c321a5bd1ecaa2433a5d49bd7...

bors · 2023-02-04T15:17:24Z

💔 Test failed - checks-actions

JulianKnodt · 2023-02-05T22:17:08Z

I've just turned wasm off on the test for now... either way it should be fine since both are correct. I forget if there were other issues that cropped up outside of wasm

oops, android was also failing for some reason so I should look into that

Still need to make it so that it maps discriminants to variant indexes. Maybe instead I can map the variant indexes to discriminants?

Changing a bunch of struct constructors to `from`, no extra destructuring, getting the type of the discriminant.

Since we're changing a bunch of stuff, necessary to remove some codegen tests which look for specific things. Also attempting to restart a test which timed out, maybe due to fastly failing?

Instead of storing an extra array for discriminant values, create an allocation there and store those in an allocation immediately.

There is a distinction between running this on wasm and i686, even though they should be identical. This technically is not _incorrect_, it's just an unexpected difference, which is worth investigating, but not for correctness.

JulianKnodt · 2023-02-08T05:48:42Z

Should be ok to retry submitting again at this point

cjgillot · 2023-02-10T21:01:14Z

@bors r+

bors · 2023-02-10T21:01:16Z

📌 Commit 15d4728 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-02-10T21:49:34Z

⌛ Testing commit 15d4728 with merge 5a8dfd9...

bors · 2023-02-11T00:50:54Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 5a8dfd9 to master...

rust-timer · 2023-02-11T02:45:19Z

Finished benchmarking commit (5a8dfd9): 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)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	1

Cycles

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

RalfJung · 2026-03-25T15:33:27Z


 impl<'tcx> MirPass<'tcx> for EnumSizeOpt {
+ fn is_enabled(&self, sess: &Session) -> bool {
+ sess.opts.unstable_opts.unsound_mir_opts || sess.mir_opt_level() >= 3


I think this commit changed the logic accidentally? This should have been unsound_mir_opts && level >= 3. Otherwise this still gets enabled despite being unsound!
I'll make a PR.

This comment has been minimized.
Sign in to view

LingMan reviewed May 11, 2021
View reviewed changes

Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated

scottmcm reviewed May 11, 2021
View reviewed changes

Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated

JulianKnodt force-pushed the array_const_val branch from 28da2d2 to 8d8c374Compare May 11, 2021 15:08

tmiasko reviewed May 12, 2021
View reviewed changes

Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated
Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated
Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated
Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated

JulianKnodt force-pushed the array_const_val branch from 8d8c374 to b65bb73Compare May 12, 2021 15:38

tmiasko reviewed May 12, 2021
View reviewed changes

Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated
Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated
Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated

JulianKnodt force-pushed the array_const_val branch from b65bb73 to 6eaa543Compare May 12, 2021 22:28

tmiasko reviewed May 13, 2021
View reviewed changes

Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 28, 2021

camelid assigned tmiasko May 28, 2021

camelid added the A-mir-opt Area: MIR optimizations label May 28, 2021

JulianKnodt force-pushed the array_const_val branch from 6eaa543 to d6b74beCompare May 28, 2021 04:18

This comment has been minimized.
Sign in to view

JulianKnodt force-pushed the array_const_val branch from d6b74be to 90f8c1cCompare May 28, 2021 04:37

This comment has been minimized.
Sign in to view

JulianKnodt force-pushed the array_const_val branch from 90f8c1c to ad67765Compare May 28, 2021 04:51

tmiasko reviewed May 28, 2021
View reviewed changes

Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated

tmiasko reviewed May 28, 2021
View reviewed changes

Comment threadcompiler/rustc_mir/src/transform/large_enums.rs Outdated

This comment has been minimized.
Sign in to view

cjgillot reviewed Feb 5, 2023
View reviewed changes

Comment threadcompiler/rustc_mir_transform/src/lib.rs Outdated
Comment threadtests/mir-opt/enum_opt.cand.EnumSizeOpt.64bit.diff Outdated

This comment has been minimized.
Sign in to view

JulianKnodt added 8 commits February 7, 2023 09:37

Create initial version of opt
18144b6

Add comments
96db5e9
Still need to make it so that it maps discriminants to variant indexes. Maybe instead I can map the variant indexes to discriminants?

Clean up MIR transform
33b4d20

Update with comments
f7cbf2e
Changing a bunch of struct constructors to `from`, no extra destructuring, getting the type of the discriminant.

Set mir-opt-level = 0 on some codegen tests
3e97cef
Since we're changing a bunch of stuff, necessary to remove some codegen tests which look for specific things. Also attempting to restart a test which timed out, maybe due to fastly failing?

Rm allocation in candidate
5d9f514
Instead of storing an extra array for discriminant values, create an allocation there and store those in an allocation immediately.

Add tag for ignoring wasm
610e1a1

Leave FIXME for wasm layout difference.
15f4eec
There is a distinction between running this on wasm and i686, even though they should be identical. This technically is not _incorrect_, it's just an unexpected difference, which is worth investigating, but not for correctness.

cjgillot reviewed Feb 7, 2023
View reviewed changes

Comment threadcompiler/rustc_mir_transform/src/large_enums.rs Outdated

Add de-init to destination place
15d4728

jackh726 mentioned this pull request Feb 15, 2023
Implement deferred_projection_equality for erica solver #107507
Merged

matthiaskrgr mentioned this pull request Nov 14, 2023
ICE: resolve: unreachable!()#117920
Closed

saethlin mentioned this pull request Nov 25, 2023
Segfault from mir-opt-level >= 3 (EnumSizeOpt) #118283
Closed

RalfJung reviewed Mar 25, 2026
View reviewed changes

RalfJung mentioned this pull request Mar 26, 2026
large_enums (EnumSizeOpt) MIR pass is unsound #154413
Open

Uh oh!

Uh oh!

Conversation

JulianKnodt commented May 10, 2021

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

tmiasko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmiasko commented May 12, 2021

Uh oh!

scottmcm commented May 12, 2021

Uh oh!

JulianKnodt commented May 12, 2021

Uh oh!

tmiasko commented May 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JulianKnodt commented May 12, 2021

Uh oh!

bjorn3 commented May 13, 2021

Uh oh!

Uh oh!

tmiasko commented May 13, 2021

Uh oh!

camelid commented May 28, 2021

Uh oh!

JulianKnodt commented May 28, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

tmiaskoMay 28, 2021

Choose a reason for hiding this comment

Uh oh!

JulianKnodtJun 9, 2021

Choose a reason for hiding this comment

Uh oh!

tmiasko commented May 28, 2021

Uh oh!

Uh oh!

Uh oh!

oli-obk commented Feb 4, 2023

Uh oh!

oli-obk commented Feb 4, 2023

Uh oh!

bors commented Feb 4, 2023

Uh oh!

bors commented Feb 4, 2023

Uh oh!

bors commented Feb 4, 2023

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

JulianKnodt commented Feb 5, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

JulianKnodt commented Feb 8, 2023

Uh oh!

cjgillot commented Feb 10, 2023

Uh oh!

bors commented Feb 10, 2023

Uh oh!

bors commented Feb 10, 2023

Uh oh!

bors commented Feb 11, 2023

Uh oh!

rust-timer commented Feb 11, 2023

Overall result: no relevant changes - no action needed

Instruction count

JulianKnodt commented Feb 5, 2023•
edited
Loading