Sink unused const assignments #106613

saethlin · 2023-01-09T02:40:59Z

Waiting for #107009 to land before re-evaluating the impact of this pass

This MIR optimization is designed to optimize away the drop flags in Option::map. It is based on the observation that there are actually two distinct code paths through Option::map, one for Option::None and the other for Option::Some, and along each of those code paths the drop flag is a constant assigned once to a local then branched on, but ConstGoto doesn't eliminate the branches because the assignments to the drop flag local don't occur in the branch immediately before the branch.

So this optimization pushes the assignments later so that when possible they appear immediately before they are branched on, and ConstGoto is able to eliminate them.

A totally viable alternative approach would be making ConstGoto smarter. But I personally find viewing this whole situation from the perspective of the SwitchInt looking backwards complicated. Worded this way, a poor version of this implementation can be implemented in a single pass over the Body which should mitigate any runaway runtime.

~~cc @JakobDegen I think this is the 2 funclets ICE~~ ICE has been fixed

rustbot · 2023-01-09T02:41:04Z

r? @oli-obk

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

erikdesjardins · 2023-01-10T00:34:01Z

compiler/rustc_mir_transform/src/const_goto.rs

@@ -28,7 +28,7 @@ pub struct ConstGoto;

 impl<'tcx> MirPass<'tcx> for ConstGoto {
    fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
-        sess.mir_opt_level() >= 4
+        sess.mir_opt_level() >= 2


It seems like DelayConstAssignments + ConstProp + SimplifyCfg ought to be able to handle everything that ConstGoto used to. (Which would be good, because those optimizations are more general than ConstGoto.)

e.g. for the simple example in ConstGoto's header:

bb2: { _2 = const true; goto -> bb3; } bb3: { switchInt(_2) -> [false: bb4, otherwise: bb5]; }

I'd expect DelayConstAssignments to change it to

bb2: { goto -> bb3; } bb3: { _2 = const true; switchInt(_2) -> [false: bb4, otherwise: bb5]; }

which ConstProp/SimplifyCfg should be able to handle.

Do you know of any situations where this doesn't work?

In case it's not obvious I have no formal training or education of any kind in compilers or optimizations. I'm just looking through the MIR that comes out of the compiler today for things that look derpy and trying to fix them.

I think before ConstProp we can have code like this:

bb0: { _8 = const 1; _9 = const 2; _10 = Add(move _8, _move 9); switchInt(move _3) -> [0: bb1, 1: bb3, otherwise: bb2]; }

And here I am not sure the amount of analysis we'd need to figure out if _10 is being assigned a const is simpler than just doing ConstProp.

Aside from the naming, it wasn't obvious. This pass is sensible :)

I may not have been clear--I think we should be able to remove ConstGoto (not ConstProp) after this change, because SinkConstAssignments should move the const assignment right into the block with the switchInt, which ConstProp/SimplifyCfg can handle without ConstGoto. Can you try your motivating Option::map example without ConstGoto, and see if it still gets optimized? Ah, I thought ConstGoto was more limited than it is--it allows multiple predecessors, so it can't be replaced like that. This is fine as-is then. (FWIW, the optimization that ConstGoto performs is referred to elsewhere as jump threading.)

It's a bit unfortunate that DataflowConstProp doesn't help here. It would know the value of the constant in the predecessor block, but there aren't any usages in the predecessor, so it doesn't do anything that would help ConstGoto. Maybe if it was available to other passes as an analysis...

compiler/rustc_mir_transform/src/delay_const_assignments.rs

erikdesjardins · 2023-01-11T08:09:12Z

compiler/rustc_mir_transform/src/sink_const_assignments.rs

+            let block_data = &body.basic_blocks[block];
+            let Some(terminator) = &block_data.terminator else { continue; };
+
+            let mut successors = Vec::new();


Perhaps this should bail out when there are too many successors. It's probably not worth creating 100 new assign statements for a 100-arm match

bors · 2023-01-26T03:51:40Z

☔ The latest upstream changes (presumably #107309) made this pull request unmergeable. Please resolve the merge conflicts.

saethlin · 2023-01-26T22:23:06Z

@bors try @rust-timer queue

bors · 2023-01-26T22:23:14Z

⌛ Trying commit 118865fb355b6535d79d52d806c3ccfb396b7ee9 with merge 90673fadd0ffde09dfdff3444aeaea5eb51b2281...

bors · 2023-01-27T01:06:48Z

☀️ Try build successful - checks-actions
Build commit: 90673fadd0ffde09dfdff3444aeaea5eb51b2281 (90673fadd0ffde09dfdff3444aeaea5eb51b2281)

rust-timer · 2023-01-27T02:42:27Z

Finished benchmarking commit (90673fadd0ffde09dfdff3444aeaea5eb51b2281): 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)	1.0%	[0.3%, 2.1%]	19
Regressions ❌ (secondary)	3.0%	[0.3%, 17.6%]	19
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.2%]	10
Improvements ✅ (secondary)	-0.7%	[-1.1%, -0.3%]	11
All ❌✅ (primary)	0.5%	[-0.9%, 2.1%]	29

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.2%	[1.0%, 3.2%]	3
Regressions ❌ (secondary)	3.9%	[1.1%, 8.3%]	10
Improvements ✅ (primary)	-4.0%	[-7.7%, -0.2%]	4
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-1.4%	[-7.7%, 3.2%]	7

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.9%	[1.2%, 2.8%]	8
Regressions ❌ (secondary)	6.8%	[2.0%, 17.3%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[1.2%, 2.8%]	8

bors · 2023-01-28T03:52:28Z

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

rust-timer · 2023-01-28T05:10:54Z

Finished benchmarking commit (f1f15ac48700d773dd71238c8f152c29dffc8b82): 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.4%	[0.3%, 0.5%]	6
Regressions ❌ (secondary)	0.6%	[0.4%, 1.2%]	13
Improvements ✅ (primary)	-0.6%	[-1.0%, -0.2%]	13
Improvements ✅ (secondary)	-0.6%	[-0.7%, -0.4%]	3
All ❌✅ (primary)	-0.3%	[-1.0%, 0.5%]	19

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)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.5%	[-7.0%, -0.2%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.0%	[-7.0%, 3.1%]	5

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.3%	[2.3%, 4.2%]	2
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

saethlin · 2023-01-28T06:41:12Z

@bors try @rust-timer queue

bors · 2023-01-28T06:41:21Z

⌛ Trying commit ac7531b7435eb8fe6b5adc29590de1b3d43a7f52 with merge 90c99bf540820d70bcb15c5c75996422aaa3ec35...

bors · 2023-01-28T09:09:23Z

☀️ Try build successful - checks-actions
Build commit: 90c99bf540820d70bcb15c5c75996422aaa3ec35 (90c99bf540820d70bcb15c5c75996422aaa3ec35)

rust-timer · 2023-01-28T10:28:36Z

Finished benchmarking commit (90c99bf540820d70bcb15c5c75996422aaa3ec35): comparison URL.

Overall result: ❌ regressions - 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.5%	[0.2%, 1.1%]	41
Regressions ❌ (secondary)	0.6%	[0.2%, 1.2%]	14
Improvements ✅ (primary)	-0.5%	[-0.7%, -0.3%]	4
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	1
All ❌✅ (primary)	0.4%	[-0.7%, 1.1%]	45

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.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-3.6%	[-6.7%, -0.4%]	7
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-2.8%	[-6.7%, 2.8%]	8

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

bors · 2023-01-29T17:16:10Z

☔ The latest upstream changes (presumably #106908) made this pull request unmergeable. Please resolve the merge conflicts.

WaffleLapkin · 2023-02-24T12:00:47Z

@rustbot author
(since this is a draft + has a merge conflict)

saethlin · 2023-03-16T22:42:59Z

I'm planning on waiting for #107009 to land before evaluating the value of landing this

rust-log-analyzer · 2023-03-16T23:05:19Z

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:24cb9080177205b6e8c946b17badbe402adc938f)
Download action repository 'rust-lang/simpleinfra@master' (SHA:3fb2b44a4eaebb9ed8086446bde46c27199ef5ed)
Complete job name: PR (mingw-check-tidy, true, ubuntu-20.04-xl)
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180119 sha256=9fa76c45f3193f307e02ea67d1a48cfe7ef2b854a074b766938a88e046bc7887
  Stored in directory: /tmp/pip-ephem-wheel-cache-nfxvsvb9/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully built a2f0d85908fc
Successfully tagged rust-ci:latest
Built container sha256:a2f0d85908fc5ec8e8f6beb64f73ca48684e2b44602e97b013dd0d647e13e7c9
Uploading finished image to https://ci-caches.rust-lang.org/docker/38eba79578bd6d379f830cfbe873c54dad8d10db1dfcf4fbac8af27e25e93a7ecaa2a7c62028c216d50741db91fde3d04f83489b4d576287b0239d35dee6e197
upload failed: - to s3://rust-lang-ci-sccache2/docker/38eba79578bd6d379f830cfbe873c54dad8d10db1dfcf4fbac8af27e25e93a7ecaa2a7c62028c216d50741db91fde3d04f83489b4d576287b0239d35dee6e197 Unable to locate credentials
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
    Finished release [optimized] target(s) in 16.86s
fmt check
tidy check
tidy: Skipping binary file check, read-only filesystem
tidy error: the following output file is not associated with any mir-opt test, you can remove it: /checkout/tests/mir-opt/lower_array_len_e2e.array_bound_mut.PreCodegen.after.mir
some tidy checks failed

bors · 2023-04-07T13:16:09Z

☔ The latest upstream changes (presumably #102906) made this pull request unmergeable. Please resolve the merge conflicts.

saethlin · 2023-10-28T05:32:13Z

Per the PR description, the jump threading pass has landed and since it produces the optimizations I was looking for with this pass, I'm closing this.

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

saethlin mentioned this pull request Jan 9, 2023

Document wf constraints on control flow in cleanup blocks #106612

Merged

This comment has been minimized.

Sign in to view

erikdesjardins reviewed Jan 10, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

erikdesjardins reviewed Jan 11, 2023

View reviewed changes

saethlin changed the title ~~Delay const assignments when unused~~ Sink unused const assignments Jan 17, 2023

cjgillot added the A-mir-opt Area: MIR optimizations label Jan 21, 2023

saethlin force-pushed the delay-const-assignments branch from 539d00c to c7d9505 Compare January 23, 2023 05:11

This comment has been minimized.

Sign in to view

saethlin mentioned this pull request Jan 26, 2023

ConstGoto can produce invalid control flow in cleanup blocks #107315

Closed

saethlin force-pushed the delay-const-assignments branch from 72f54b6 to c445d7e Compare January 26, 2023 05:42

This comment has been minimized.

Sign in to view

saethlin force-pushed the delay-const-assignments branch 2 times, most recently from 80a7213 to 118865f Compare January 26, 2023 14:52

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 Jan 26, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 27, 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 Jan 28, 2023

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 Jan 28, 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 Jan 28, 2023

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2023

saethlin force-pushed the delay-const-assignments branch from a171f45 to 8396fc3 Compare March 16, 2023 22:40

Create SinkConstAssignments pass

0fc3e10

saethlin force-pushed the delay-const-assignments branch from 8396fc3 to 0fc3e10 Compare March 16, 2023 22:46

Dylan-DPC added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2023

saethlin closed this Oct 28, 2023

saethlin deleted the delay-const-assignments branch October 28, 2023 05:32

saethlin removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sink unused const assignments #106613

Sink unused const assignments #106613

saethlin commented Jan 9, 2023 •

edited

Loading

rustbot commented Jan 9, 2023

This comment has been minimized.

erikdesjardins Jan 10, 2023

saethlin Jan 11, 2023

erikdesjardins Jan 11, 2023

This comment was marked as resolved.

This comment was marked as resolved.

erikdesjardins Jan 11, 2023

This comment has been minimized.

bors commented Jan 26, 2023

This comment has been minimized.

saethlin commented Jan 26, 2023

This comment has been minimized.

bors commented Jan 26, 2023

bors commented Jan 27, 2023

This comment has been minimized.

rust-timer commented Jan 27, 2023

bors commented Jan 28, 2023

This comment has been minimized.

rust-timer commented Jan 28, 2023

saethlin commented Jan 28, 2023

This comment has been minimized.

bors commented Jan 28, 2023

bors commented Jan 28, 2023

This comment has been minimized.

rust-timer commented Jan 28, 2023

bors commented Jan 29, 2023

WaffleLapkin commented Feb 24, 2023

saethlin commented Mar 16, 2023

rust-log-analyzer commented Mar 16, 2023

bors commented Apr 7, 2023

saethlin commented Oct 28, 2023

Sink unused const assignments #106613

Sink unused const assignments #106613

Conversation

saethlin commented Jan 9, 2023 • edited Loading

rustbot commented Jan 9, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Jan 26, 2023

This comment has been minimized.

saethlin commented Jan 26, 2023

This comment has been minimized.

bors commented Jan 26, 2023

bors commented Jan 27, 2023

This comment has been minimized.

rust-timer commented Jan 27, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

bors commented Jan 28, 2023

This comment has been minimized.

rust-timer commented Jan 28, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

saethlin commented Jan 28, 2023

This comment has been minimized.

bors commented Jan 28, 2023

bors commented Jan 28, 2023

This comment has been minimized.

rust-timer commented Jan 28, 2023

Overall result: ❌ regressions - ACTION NEEDED

bors commented Jan 29, 2023

WaffleLapkin commented Feb 24, 2023

saethlin commented Mar 16, 2023

rust-log-analyzer commented Mar 16, 2023

bors commented Apr 7, 2023

saethlin commented Oct 28, 2023

saethlin commented Jan 9, 2023 •

edited

Loading