gvn: Promote/propagate const local array #126444

tesuji · 2024-06-13T22:01:58Z

Rewriting of #125916 which used PromoteTemps pass.

This allows promoting constant local arrays as anonymous constants. So that's in codegen for
a local array, rustc outputs llvm.memcpy (which is easy for LLVM to optimize) instead of a series
of store on stack (a.k.a in-place initialization). This makes rustc on par with clang on this specific case.
See more in #73825 or zulip for more info.

Here is a simple micro benchmark that shows the performance differences between promoting arrays or not.

Prior discussions on zulip.

This patch saves about 600 KB (~0.5%) of librustc_driver.so.

Fix #73825

r? cjgillot

Unresolved questions

Should we ignore nested arrays?
I think that promoting nested arrays is bloating codegen.
Should stack_threshold be at least 32 bytes? Like the benchmark showed.
If yes, the test should be updated to make arrays larger than 32 bytes.
~~Is this concerning that call(move _1) is now call(const [array])?~~
It reverted back to call(move _1)

rustbot · 2024-06-13T22:02:01Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_const_eval/src/interpret/place.rs

compiler/rustc_mir_transform/src/gvn.rs

jieyouxu · 2024-06-13T22:52:18Z

@bors try @rust-timer queue

[WIP] gvn: Promote/propagate const local array Rewriting of rust-lang#125916 which used PromoteTemps pass. Fix rust-lang#73825 ### Current status - [ ] Waiting for [consensus](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Could.20const.20read-only.20arrays.20be.20const.20promoted.3F). r? ghost

bors · 2024-06-13T22:53:30Z

⌛ Trying commit 550fb81 with merge e26c0b3...

bors · 2024-06-14T00:31:34Z

☀️ Try build successful - checks-actions
Build commit: e26c0b3 (e26c0b3f8c9af007281a11df56a0bf825d8b4cb0)

rust-timer · 2024-06-14T01:45:17Z

Finished benchmarking commit (e26c0b3): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.1%)

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)	9.2%	[4.2%, 18.7%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-9.1%	[-9.9%, -8.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[-9.9%, 18.7%]	6

Cycles

Results (primary -4.0%)

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)	-4.0%	[-5.3%, -1.7%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-4.0%	[-5.3%, -1.7%]	5

Binary size

Results (primary -0.1%, secondary 0.0%)

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.0%	[0.0%, 0.0%]	1
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.2%, 0.0%]	6

Bootstrap: 671.518s -> 674.73s (0.48%)
Artifact size: 320.38 MiB -> 320.39 MiB (0.00%)

Kobzol · 2024-06-14T13:51:30Z

@bors try @rust-timer queue

bors · 2024-06-14T13:52:42Z

⌛ Trying commit 6c6de58 with merge 7e160d4...

[WIP] gvn: Promote/propagate const local array Rewriting of rust-lang#125916 which used PromoteTemps pass. Fix rust-lang#73825 ### Current status - [ ] Waiting for [consensus](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Could.20const.20read-only.20arrays.20be.20const.20promoted.3F). r? ghost

bors · 2024-06-14T15:25:23Z

☀️ Try build successful - checks-actions
Build commit: 7e160d4 (7e160d4b55bb5a27be0696f45db247ccc2e166d9)

rust-timer · 2024-06-14T16:42:14Z

Finished benchmarking commit (7e160d4): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.7%)

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)	11.0%	[3.9%, 21.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-7.6%	[-10.4%, -4.8%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.7%	[-10.4%, 21.3%]	6

Cycles

Results (secondary 9.0%)

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)	9.0%	[9.0%, 9.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.1%)

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.0%	[0.0%, 0.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.2%, 0.0%]	6

Bootstrap: 670.938s -> 673.147s (0.33%)
Artifact size: 320.39 MiB -> 319.79 MiB (-0.19%)

big arrays make the diff hard to read

tesuji · 2024-06-15T09:31:06Z

How does it affect debug build ? Isn't gvn run only if mir_opt_level >= 2

Kobzol · 2024-06-15T10:41:39Z

Don't mind max RSS, it's quite noisy. Unless there are regressions across the board, you can mostly ignore it.

this slips through commmit "shorten array in tests"

compiler/rustc_mir_transform/src/gvn.rs

cjgillot · 2024-06-16T14:31:38Z

compiler/rustc_mir_transform/src/gvn.rs

@@ -382,7 +396,16 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
                let ty = match kind {
                    AggregateTy::Array => {
                        assert!(fields.len() > 0);
-                        Ty::new_array(self.tcx, fields[0].layout.ty, fields.len() as u64)
+                        let field_ty = fields[0].layout.ty;
+                        // Ignore nested array


Could you write why?

I just feel that nested arrays are rarer than 1-demensional arrays.
And it is kinda redundant that in MIR output that each Local is const promoted as a const array.

If nested array are allowed, we would have.

let a = [[b], [c]];

promoted as

_b = const [.. ]; _c = const [.. ]; -_a = const [move _b, moved _c]; +_a = const [[..], [..]];

We could const promote only _a, but it is more complex to ignore _b and _c.

cjgillot · 2024-06-16T14:32:49Z

compiler/rustc_mir_transform/src/gvn.rs

+                    if ty.layout.size().bytes() <= STACK_THRESHOLD {
+                        return None;
+                    }


For this case, should we attempt to fallback to the next branch?

But the fallback is return None at line L471 ? Maybe I read the code wrong.

I mean the matches!(ty.abi, Abi::Scalar(..) | Abi::ScalarPair(..)) branch.

But ty.abi of array is not Abi::Scalar* though. And with STACK_THRESHOLD, I fallback to default behavior on master branch. Also I committed 4fb498e.
But I don't get what the differences between ecx.intern_with_temp_alloc and ecx.alloc, in the case of big array (sizeof(array) > stack_size). Or should I use rustc_const_eval::const_eval::machine::MemoryKind?

compiler/rustc_mir_transform/src/gvn.rs

cjgillot · 2024-06-16T14:44:49Z

compiler/rustc_mir_transform/src/gvn.rs

+            // Ignore arrays in operands.
+            // Prevent code bloat that makes
+            // `_2 = _1` now resolved to `_2 = <evaluated array>`.
+            let disallow_dup_array = if rvalue.ty(self.local_decls, self.tcx).is_array()
+                && let Some(locals) = self.rev_locals.get(value).as_deref()
+                && let [first, ..] = locals[..]
+            {
+                first != lhs.local
+            } else {
+                false
+            };


I don't understand what this is meant to do. Why is this only checking the first inside rev_local?

Without it, duplicate arrays in code will be duplicated in mir output.
Consider this

// code a = [1,2,3]; b = [1,2,3]; // mir with and without this condition. _1 = const [1,2,3]; -_2 = _1; +_2 = const [1,2,3];

In short, we want to run try_as_local for duplicate arrays. But I don't know how often it is to worry about it.

tests/codegen/issue-73825-gvn-const-local-array.rs

tests/mir-opt/const_array_locals.rs

cjgillot · 2024-06-16T14:48:28Z

tests/mir-opt/const_array_locals.main.GVN.diff

+          StorageDead(_12);
+          StorageLive(_13);
+          StorageLive(_14);
+          _14 = [const 1_i32, const 0_i32, const 0_i32];


Why isn't this one made a constant?

The sizeof(_14) is 12 bytes ( < STACK_THRESHOULD). After codegen, it is just a mov xmm instruction, which is very cheap.

Co-authored-by: Camille Gillot <gillot.camille@gmail.com>

more explicit ir checks

dbg: move to sub functions for easier filtering tracing

This breaks many invariants of the pass. `new_index` may not be newly created. The caching is what `evaluated` does.

rustbot · 2024-07-07T00:25:28Z

Could not assign reviewer from: cjgillot.
User(s) cjgillot are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

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 Jun 13, 2024

scottmcm reviewed Jun 13, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/place.rs Outdated Show resolved Hide resolved

scottmcm reviewed Jun 13, 2024

View reviewed changes

compiler/rustc_mir_transform/src/gvn.rs Outdated Show resolved Hide resolved

tesuji changed the title ~~WIP: Trying to promote const local array using GVN pass~~ WIP: Promote/propagate const local array using GVN pass Jun 13, 2024

tesuji force-pushed the gvn-const-arrays branch from e8f832f to 5944765 Compare June 13, 2024 22:32

tesuji changed the title ~~WIP: Promote/propagate const local array using GVN pass~~ [WIP] gvn: Promote/propagate const local array Jun 13, 2024

This comment has been minimized.

Sign in to view

tesuji force-pushed the gvn-const-arrays branch from 5944765 to 550fb81 Compare June 13, 2024 22:43

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 Jun 13, 2024

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 Jun 14, 2024

tesuji mentioned this pull request Jun 14, 2024

[WIP] mir-opt: promoting const read-only arrays #125916

Closed

4 tasks

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 Jun 14, 2024

This comment has been minimized.

Sign in to view

rm unneeded checks

255f690

tesuji force-pushed the gvn-const-arrays branch from 4009326 to 255f690 Compare June 14, 2024 21:20

rustbot added the A-mir-opt Area: MIR optimizations label Jun 14, 2024

This comment has been minimized.

Sign in to view

tesuji added 2 commits June 15, 2024 00:38

bless 32bit diff

5837acd

shorten array in tests

108c477

big arrays make the diff hard to read

tesuji added 3 commits June 15, 2024 11:26

ignore arrays in operands

42f61e5

disallow duplicate arrays in rvalue

bba3ddb

adjust test for call(array)

07aa65b

this slips through commmit "shorten array in tests"

cjgillot reviewed Jun 16, 2024

View reviewed changes

Apply suggestions from code review

d04dbd0

Co-authored-by: Camille Gillot <gillot.camille@gmail.com>

This comment has been minimized.

Sign in to view

tesuji added 8 commits June 16, 2024 16:45

fixup! add codegen test for issue 73825

79e99d6

more explicit ir checks

add explicit mir checks for mir-opt test

164bd5b

amend! dbg: move to sub functions for easier filtering tracing

41accf8

dbg: move to sub functions for easier filtering tracing

explain why we ignore nested arrays

735cb46

Explain code with more comments

b2e6fa6

rm some debug!

4883b26

Revert "cache propagated const"

2550603

This breaks many invariants of the pass. `new_index` may not be newly created. The caching is what `evaluated` does.

fixup! Explain code with more comments

a9d6041

This comment has been minimized.

Sign in to view

tesuji added 2 commits June 16, 2024 22:13

fixup! Revert "cache propagated const"

5c0ccaf

replace ecx.intern_with_temp_alloc with ecx.alloc

4fb498e

rustbot assigned oli-obk and unassigned cjgillot Jul 4, 2024

oli-obk assigned cjgillot and unassigned oli-obk Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gvn: Promote/propagate const local array #126444

gvn: Promote/propagate const local array #126444

tesuji commented Jun 13, 2024 •

edited

Loading

rustbot commented Jun 13, 2024

This comment has been minimized.

jieyouxu commented Jun 13, 2024

This comment has been minimized.

bors commented Jun 13, 2024

This comment has been minimized.

bors commented Jun 14, 2024

This comment has been minimized.

rust-timer commented Jun 14, 2024

This comment has been minimized.

Kobzol commented Jun 14, 2024

This comment has been minimized.

bors commented Jun 14, 2024

bors commented Jun 14, 2024

This comment has been minimized.

rust-timer commented Jun 14, 2024

This comment has been minimized.

tesuji commented Jun 15, 2024 •

edited

Loading

Kobzol commented Jun 15, 2024

cjgillot Jun 16, 2024

tesuji Jun 16, 2024 •

edited

Loading

cjgillot Jun 16, 2024

tesuji Jun 16, 2024

cjgillot Jun 17, 2024

tesuji Jun 17, 2024 •

edited

Loading

cjgillot Jun 16, 2024

tesuji Jun 16, 2024 •

edited

Loading

cjgillot Jun 16, 2024

tesuji Jun 16, 2024

This comment has been minimized.

This comment has been minimized.

rustbot commented Jul 7, 2024

gvn: Promote/propagate const local array #126444

Are you sure you want to change the base?

gvn: Promote/propagate const local array #126444

Conversation

tesuji commented Jun 13, 2024 • edited Loading

Unresolved questions

rustbot commented Jun 13, 2024

This comment has been minimized.

jieyouxu commented Jun 13, 2024

This comment has been minimized.

bors commented Jun 13, 2024

This comment has been minimized.

bors commented Jun 14, 2024

This comment has been minimized.

rust-timer commented Jun 14, 2024

Overall result: no relevant changes - no action needed

This comment has been minimized.

Kobzol commented Jun 14, 2024

This comment has been minimized.

bors commented Jun 14, 2024

bors commented Jun 14, 2024

This comment has been minimized.

rust-timer commented Jun 14, 2024

Overall result: no relevant changes - no action needed

This comment has been minimized.

tesuji commented Jun 15, 2024 • edited Loading

Kobzol commented Jun 15, 2024

Choose a reason for hiding this comment

tesuji Jun 16, 2024 • edited Loading

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

tesuji Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tesuji Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

rustbot commented Jul 7, 2024

tesuji commented Jun 13, 2024 •

edited

Loading

tesuji commented Jun 15, 2024 •

edited

Loading

tesuji Jun 16, 2024 •

edited

Loading

tesuji Jun 17, 2024 •

edited

Loading

tesuji Jun 16, 2024 •

edited

Loading