Strengthen state tracking in const-prop #108872

cjgillot · 2023-03-07T17:12:17Z

Some/many of the changes are replicated between both the const-prop lint and the const-prop optimization.

Behaviour changes:

const-prop opt does not give a span to propagated values. This was useless as that span's primary purpose is to diagnose evaluation failure in codegen.
we remove the OnlyPropagateInto mode. It was only used for function arguments, which are better modeled by a write before entry.
the tracking of assignments and discriminants make clearer that we do nothing in NoPropagation mode or on indirect places.

rustbot · 2023-03-07T17:12:22Z

r? @davidtwco

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

rustbot · 2023-03-07T17:12:26Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustbot · 2023-03-07T17:53:43Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2023-03-08T09:00:10Z

compiler/rustc_mir_transform/src/const_prop.rs

-                        source_info.span,
-                    ));
+                    *rval =
+                        Rvalue::Use(self.operand_from_scalar(scalar, value.layout.ty, DUMMY_SP));


operand_from_scalar could just stop taking a span alltogether

oli-obk · 2023-03-08T13:22:09Z

r=me with operand_from_scalar havings its span arg removed

oli-obk · 2023-03-08T14:49:31Z

@bors r+ rollup=never

bors · 2023-03-08T14:49:33Z

📌 Commit 8179b2e has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-03-12T23:27:56Z

⌛ Testing commit 8179b2e with merge b05bb29...

bors · 2023-03-13T02:17:34Z

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

rust-timer · 2023-03-13T03:41:15Z

Finished benchmarking commit (b05bb29): 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)	2.4%	[0.6%, 6.3%]	30
Regressions ❌ (secondary)	41.6%	[0.3%, 139.3%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.9%, -1.0%]	2
All ❌✅ (primary)	2.4%	[0.6%, 6.3%]	30

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)	-2.2%	[-2.6%, -1.5%]	4
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	1

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.4%	[0.9%, 1.6%]	5
Regressions ❌ (secondary)	21.5%	[3.9%, 45.4%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[0.9%, 1.6%]	5

saethlin · 2023-03-13T04:49:43Z

compiler/rustc_mir_transform/src/const_prop.rs

+        for (local, &mode) in can_const_prop.iter_enumerated() {
+            match mode {
+                ConstPropMode::FullConstProp => {}


It looks to me like the regression is from this loop? At least that's what my profiler indicates. It looks like basically all the cycles in ConstProp are driving this iterator and hitting this first case:

16.79 │ d0:┌─→inc %rdi │ │<core::slice::iter::Iter<rustc_mir_transform::const_prop::ConstPropMode> as core::iter::traits::iterator::Iterator>::next: 16.05 │ │ add $0x48,%rdx 16.52 │ │ cmp %rdi,%rax │ │↓ je 12e │ │<rustc_middle::mir::Local>::from_usize: │ │} │ │ │ │/////////////////////////////////////////////////////////////////////////// │ │// Variables and temps │ │ │ │rustc_index::newtype_index! { 25.17 │ dc:│ cmp %r8,%rdi │ │↓ je 1b7 │ │<rustc_mir_transform::const_prop::ConstPropagator as rustc_middle::mir::visit::MutVisitor>::visit_basic_block_data: │ │match mode { 25.18 │ ├──cmpb $0x1,(%rcx,%rdi,1) │ └──jne d0

I will not be working on this, but I suspect the problem centers around there being a lot of FullConstProp in here and we could be iterating over less elements somehow.

Thanks for the analysis. I'll handle it after hours.

Only clear written-to locals in ConstProp This aims to revert the regression in rust-lang#108872 Clearing all locals at the end of each bb is very costly. This PR goes back to the original implementation of recording which locals are modified.

Only clear written-to locals in ConstProp This aims to revert the regression in rust-lang/rust#108872 Clearing all locals at the end of each bb is very costly. This PR goes back to the original implementation of recording which locals are modified.

Strengthen state tracking in const-prop Some/many of the changes are replicated between both the const-prop lint and the const-prop optimization. Behaviour changes: - const-prop opt does not give a span to propagated values. This was useless as that span's primary purpose is to diagnose evaluation failure in codegen. - we remove the `OnlyPropagateInto` mode. It was only used for function arguments, which are better modeled by a write before entry. - the tracking of assignments and discriminants make clearer that we do nothing in `NoPropagation` mode or on indirect places.

Only clear written-to locals in ConstProp This aims to revert the regression in rust-lang/rust#108872 Clearing all locals at the end of each bb is very costly. This PR goes back to the original implementation of recording which locals are modified.

rustbot assigned davidtwco Mar 7, 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 Mar 7, 2023

cjgillot force-pushed the simp-const-prop branch from 7677516 to 5f5ca66 Compare March 7, 2023 17:53

oli-obk approved these changes Mar 8, 2023

View reviewed changes

davidtwco assigned oli-obk and unassigned davidtwco Mar 8, 2023

cjgillot added 13 commits March 8, 2023 14:40

Create visit_block_data for const-prop-lint.

9a56933

Simplify visit_statement.

2247cd6

Do not track span in ConstProp.

0e64ce7

Assume the frame has all the locals.

081bc75

Only assign value in remove_const.

24dbf9c

Remove OnlyPropagateInto.

9928d0e

Refactor tracking of writes.

d97a7ce

Recurse into statement before applying its effect.

f00be8b

Separate checking rvalue from evaluation.

b55c4f8

Make comment more explicit.

0d56034

Fortify clippy tests.

a5a01b2

Bless 32bit.

d94ec30

Remove useless parameter to operand_from_scalar.

8179b2e

cjgillot force-pushed the simp-const-prop branch from 4d89b7b to 8179b2e Compare March 8, 2023 14:42

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 Mar 8, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2023

bors merged commit b05bb29 into rust-lang:master Mar 13, 2023

rustbot added this to the 1.70.0 milestone Mar 13, 2023

rustbot added the perf-regression Performance regression. label Mar 13, 2023

saethlin reviewed Mar 13, 2023

View reviewed changes

cjgillot mentioned this pull request Mar 13, 2023

Only clear written-to locals in ConstProp #109087

Merged

cjgillot deleted the simp-const-prop branch March 13, 2023 20:24

cuviper mentioned this pull request Mar 29, 2023

const-propagated arithmetic_overflow in unreachable code #109731

Open

lqd mentioned this pull request Nov 28, 2023

ConstProp misoptimises pointer-typed enum field #118328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strengthen state tracking in const-prop #108872

Strengthen state tracking in const-prop #108872

cjgillot commented Mar 7, 2023

rustbot commented Mar 7, 2023

rustbot commented Mar 7, 2023

rustbot commented Mar 7, 2023

oli-obk Mar 8, 2023

oli-obk commented Mar 8, 2023

oli-obk commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 12, 2023

bors commented Mar 13, 2023

rust-timer commented Mar 13, 2023

saethlin Mar 13, 2023

saethlin Mar 13, 2023

cjgillot Mar 13, 2023

Strengthen state tracking in const-prop #108872

Strengthen state tracking in const-prop #108872

Conversation

cjgillot commented Mar 7, 2023

rustbot commented Mar 7, 2023

rustbot commented Mar 7, 2023

rustbot commented Mar 7, 2023

oli-obk Mar 8, 2023

Choose a reason for hiding this comment

oli-obk commented Mar 8, 2023

oli-obk commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 12, 2023

bors commented Mar 13, 2023

rust-timer commented Mar 13, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

saethlin Mar 13, 2023

Choose a reason for hiding this comment

saethlin Mar 13, 2023

Choose a reason for hiding this comment

cjgillot Mar 13, 2023

Choose a reason for hiding this comment