Do not deaggregate MIR #107267

cjgillot · 2023-01-24T17:42:45Z

This turns out to simplify a lot of things.
I haven't checked the consequences for miri yet.

cc @JakobDegen
r? @oli-obk

rustbot · 2023-01-24T17:42:54Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

cjgillot · 2023-01-24T20:25:56Z

@bors try @rust-timer queue

bors · 2023-01-24T20:26:05Z

⌛ Trying commit bf1045f6445947bde254afb44e4f51e59d5ee3cd with merge 1c5362be2bfa0958bd3e3d628499105ba64bcef2...

bors · 2023-01-24T23:09:56Z

☀️ Try build successful - checks-actions
Build commit: 1c5362be2bfa0958bd3e3d628499105ba64bcef2 (1c5362be2bfa0958bd3e3d628499105ba64bcef2)

rust-timer · 2023-01-25T00:29:32Z

Finished benchmarking commit (1c5362be2bfa0958bd3e3d628499105ba64bcef2): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.6%	[0.4%, 0.9%]	4
Regressions ❌ (secondary)	0.6%	[0.3%, 1.5%]	13
Improvements ✅ (primary)	-0.9%	[-3.0%, -0.2%]	48
Improvements ✅ (secondary)	-5.9%	[-13.4%, -0.3%]	28
All ❌✅ (primary)	-0.7%	[-3.0%, 0.9%]	52

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)	4.0%	[4.0%, 4.0%]	1
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-3.6%	[-8.4%, -0.1%]	10
Improvements ✅ (secondary)	-5.9%	[-9.6%, -1.4%]	23
All ❌✅ (primary)	-2.9%	[-8.4%, 4.0%]	11

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)	1.7%	[1.0%, 2.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.3%, -1.3%]	4
Improvements ✅ (secondary)	-7.0%	[-10.8%, -4.3%]	27
All ❌✅ (primary)	-0.8%	[-2.3%, 2.3%]	6

oli-obk · 2023-01-25T08:02:20Z

I reopened #35259 because there are conflicting desires

@eddyb would prefer to go all the way and remove Aggregate from the MIR entirely
- this avoids having two ways to do the same thing, but needs a marker for when a value is fully constructed
@JakobDegen would prefer to do this PR and never deaggregate
- this is easier on analyses and some optimizations, but now we have two ways to do the same thing (assigning each field separately vs assigning them all at once)

The discussion that caused this PR: https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/Changing.20enum.20variant.20by.20writing.20to.20field

JakobDegen · 2023-01-25T11:16:16Z

One thing we can do is keep deaggregating structs, but not enums. Then there won't be two ways of doing the same thing, as long as we don't allow SetDiscriminant for enums at all

Replace ZST operands and debuginfo by constants. This is work that ConstProp will not have to do. Split from rust-lang#107267

oli-obk · 2023-01-27T19:14:58Z

So... basically the reason deaggregation was enabled for everything but arrays was to work towards named return value optimization. Instead of building all the components and then doing a single aggregate assignment at the end, you can assign to sub-places immediately and even end up with multiple layers of function calls that assign to a place in the outermost call.

I guess we can still do that as part of nrvo when it decides a return value should get that opt run on it.

JakobDegen · 2023-01-29T00:33:12Z

So... basically the reason deaggregation was enabled for everything but arrays was to work towards named return value optimization. Instead of building all the components and then doing a single aggregate assignment at the end, you can assign to sub-places immediately and even end up with multiple layers of function calls that assign to a place in the outermost call.

Honestly, this is a problem we should fix via SSA (or something similar, not via deaggregation)

oli-obk · 2023-01-29T11:32:40Z

Makes sense. So I guess the only thing left is to give codegen backends the ability to deaggregate so they don't have to implement both aggregate rvalues, field assignments and setdiscriminant unless they want to

rust-timer · 2023-02-04T20:29:59Z

Finished benchmarking commit (9dee4e4): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.5%	[0.2%, 1.1%]	51
Regressions ❌ (secondary)	0.8%	[0.1%, 2.0%]	30
Improvements ✅ (primary)	-1.4%	[-2.6%, -0.3%]	25
Improvements ✅ (secondary)	-6.0%	[-12.9%, -0.6%]	27
All ❌✅ (primary)	-0.1%	[-2.6%, 1.1%]	76

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)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	2.3%	[1.4%, 2.6%]	5
Improvements ✅ (primary)	-3.9%	[-9.5%, -0.1%]	8
Improvements ✅ (secondary)	-6.5%	[-9.4%, -2.7%]	21
All ❌✅ (primary)	-3.2%	[-9.5%, 2.6%]	9

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.8%	[0.7%, 0.9%]	3
Regressions ❌ (secondary)	2.3%	[2.2%, 2.5%]	2
Improvements ✅ (primary)	-1.6%	[-2.3%, -1.0%]	8
Improvements ✅ (secondary)	-6.3%	[-10.4%, -1.2%]	28
All ❌✅ (primary)	-1.0%	[-2.3%, 0.9%]	11

nnethercote · 2023-02-05T22:26:22Z

The wins here outweigh the losses, even if you ignore the handful of very good results among secondary benchmarks that skew the overall results.

@rustbot label: +perf-regression-triaged

Adapt SROA MIR opt for aggregated MIR The pass was broken by rust-lang#107267. This PR extends it to replace: ``` x = Struct { 0: a, 1: b } y = move? x ``` by assignment between locals ``` x_0 = a x_1 = b y_0 = move? x_0 y_1 = move? x_1 ``` The improved pass runs to fixpoint, so we can flatten nested field accesses.

bjorn3 · 2023-02-06T17:12:02Z

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

-                        } else {
-                            (dest, active_field_index)
-                        }
+                let (variant_index, variant_dest, active_field_index) = match **kind {


Cg_clif needs to be updated too. Will do this over at the cg_clif repo.

Edit: done in bjorn3/rustc_codegen_cranelift@8494882

Manishearth · 2023-02-08T02:41:27Z

This caused a regression: #107678

Should this be reverted? (Or perhaps you can help find the bug)

saethlin · 2023-02-08T03:17:01Z

The fix is #107688, I think this PR just makes the root cause easier to access. I've seen similar-looking ICEs in other crates (associated type vs concrete type) but was never able to minimize them.

If you're curious there should be other examples like the referenced ICE here: #107051

Replace ZST operands and debuginfo by constants. This is work that ConstProp will not have to do. Split from rust-lang#107267

Replace ZST operands and debuginfo by constants. This is work that ConstProp will not have to do. Split from rust-lang/rust#107267

rustbot assigned oli-obk Jan 24, 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 Jan 24, 2023

cjgillot mentioned this pull request Jan 24, 2023

Replace ZST operands and debuginfo by constants. #107270

Merged

cjgillot force-pushed the keep-aggregate branch 3 times, most recently from 6278cd7 to bf1045f Compare January 24, 2023 20:07