Check whether locals are too large instead of whether accesses into them are too large #74821

oli-obk · 2020-07-27T11:56:44Z

Essentially this stops const prop from attempting to optimize

let mut x = [0_u8; 5000];
x[42] = 3;

I don't expect this to be a perf improvement without #73656 (which is also where the lack of this PR will be a perf regression).

r? @wesleywiser

oli-obk · 2020-07-27T13:13:49Z

@bors try @rust-timer queue

rust-timer · 2020-07-27T13:13:50Z

Awaiting bors try build completion

bors · 2020-07-27T13:14:00Z

⌛ Trying commit a9725ee777812a379d62de197d7c1bbed4cf45eb with merge bd190279c18106147d12a444e6ae7bc861b2de24...

bors · 2020-07-27T13:55:06Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: bd190279c18106147d12a444e6ae7bc861b2de24 (bd190279c18106147d12a444e6ae7bc861b2de24)

rust-timer · 2020-07-27T13:55:08Z

Queued bd190279c18106147d12a444e6ae7bc861b2de24 with parent 52d2c7a, future comparison URL.

rust-timer · 2020-07-27T17:46:20Z

Finished benchmarking try commit (bd190279c18106147d12a444e6ae7bc861b2de24): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

wesleywiser

LGTM! Can we add a mir-opt test showing that

let mut x = [0_u8; 5000];
x[42] = 3;

does not propagate? r=me with a test.

oli-obk · 2020-07-28T12:40:05Z

@bors r=wesleywiser

bors · 2020-07-28T12:40:06Z

📌 Commit 590885ade8a4363e1a7a75a0ffeed98236df8aa5 has been approved by wesleywiser

bors · 2020-07-28T12:58:09Z

⌛ Testing commit 590885ade8a4363e1a7a75a0ffeed98236df8aa5 with merge c0c2bd7f44ee27f17f8fbff023d61267636fa763...

bors · 2020-07-28T13:22:42Z

💔 Test failed - checks-actions

oli-obk · 2020-07-28T14:51:41Z

@bors r=wesleywiser

bors · 2020-07-28T14:51:42Z

📌 Commit 99a85e811f45b7aa02d917c44c12702d007b9131 has been approved by wesleywiser

bors · 2020-07-29T19:54:15Z

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

…hem are too large

We used to erase these values immediately after propagation, but some things slipped through and it caused us to still initialize huge locals.

The reason we do not trigger these lints anymore is that clippy sets the mir-opt-level to 0, and the recent changes subtly changed how the const propagator works.

oli-obk · 2020-07-29T20:41:18Z

I made some changes to the mir-opt test suite. It now prints the name of the mir dump file it's expecting and the path to the file which requested the mir dump

xldenis · 2020-07-31T14:38:18Z

src/tools/compiletest/src/runtest.rs

+        let to_full_path = |path: PathBuf| {
+            let full = self.get_mir_dump_dir().join(&path);
+            if !full.exists() {
+                panic!(


👍 there are a couple other places in check_mir_dump that also produce confusing / less useful errors but that could be fixed in a seperate pr.

wesleywiser · 2020-08-07T14:34:11Z

@bors r+

bors · 2020-08-07T14:34:12Z

📌 Commit 1864a97 has been approved by wesleywiser

bors · 2020-08-07T15:28:13Z

⌛ Testing commit 1864a97 with merge 4d43423...

bors · 2020-08-07T17:19:51Z

☀️ Test successful - checks-actions, checks-azure
Approved by: wesleywiser
Pushing 4d43423 to master...

@wesleywiser

…ath, r=wesleywiser Check whether locals are too large instead of whether accesses into them are too large Essentially this stops const prop from attempting to optimize ```rust let mut x = [0_u8; 5000]; x[42] = 3; ``` I don't expect this to be a perf improvement without rust-lang#73656 (which is also where the lack of this PR will be a perf regression). r? @wesleywiser

rust-highfive assigned wesleywiser Jul 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2020

This comment has been minimized.

Sign in to view

oli-obk mentioned this pull request Jul 27, 2020

move Deaggregate pass to post_borrowck_cleanup #73656

Merged

oli-obk force-pushed the const_eval_read_uninit_fast_path branch from b94da2b to 74ff07e Compare July 27, 2020 12:27

oli-obk changed the title ~~Check wether locals are too large instead of whether accesses into them are too large~~ Check whether locals are too large instead of whether accesses into them are too large Jul 27, 2020

oli-obk force-pushed the const_eval_read_uninit_fast_path branch from 74ff07e to a9725ee Compare July 27, 2020 13:02

wesleywiser approved these changes Jul 28, 2020

View reviewed changes

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 Jul 28, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 28, 2020

oli-obk force-pushed the const_eval_read_uninit_fast_path branch from 590885a to 99a85e8 Compare July 28, 2020 14:51

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 29, 2020

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2020

oli-obk added 7 commits July 29, 2020 22:14

Check whether locals are too large instead of whether accesses into t…

c3454c0

…hem are too large

Stop propagating to locals that were marks as unpropagatable.

b26a7d5

We used to erase these values immediately after propagation, but some things slipped through and it caused us to still initialize huge locals.

Update ui tests

9e21004

Add test ensuring that we don't propagate large arrays

8d5f2bd

Update clippy ui test.

dc0408e

The reason we do not trigger these lints anymore is that clippy sets the mir-opt-level to 0, and the recent changes subtly changed how the const propagator works.

Update tests after rebase

f7a1e64

Improve the diagnostics around misspelled mir dump filenames

1864a97

oli-obk force-pushed the const_eval_read_uninit_fast_path branch from 303547c to 1864a97 Compare July 29, 2020 20:40

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

xldenis reviewed Jul 31, 2020

View reviewed changes

wesleywiser approved these changes Aug 7, 2020

View reviewed changes

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 Aug 7, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 7, 2020

bors merged commit 4d43423 into rust-lang:master Aug 7, 2020

This was referenced Aug 7, 2020

diagnostics: shorten paths of unique symbols #73996

Merged

Miri: Renamed "undef" to "uninit" #75226

Merged

oli-obk deleted the const_eval_read_uninit_fast_path branch August 8, 2020 08:35

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check whether locals are too large instead of whether accesses into them are too large #74821

Check whether locals are too large instead of whether accesses into them are too large #74821

oli-obk commented Jul 27, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Jul 27, 2020

rust-timer commented Jul 27, 2020

bors commented Jul 27, 2020

bors commented Jul 27, 2020

rust-timer commented Jul 27, 2020

rust-timer commented Jul 27, 2020

wesleywiser left a comment

oli-obk commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

oli-obk commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 29, 2020

oli-obk commented Jul 29, 2020

xldenis Jul 31, 2020

wesleywiser commented Aug 7, 2020

bors commented Aug 7, 2020

bors commented Aug 7, 2020

bors commented Aug 7, 2020

Check whether locals are too large instead of whether accesses into them are too large #74821

Check whether locals are too large instead of whether accesses into them are too large #74821

Conversation

oli-obk commented Jul 27, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Jul 27, 2020

rust-timer commented Jul 27, 2020

bors commented Jul 27, 2020

bors commented Jul 27, 2020

rust-timer commented Jul 27, 2020

rust-timer commented Jul 27, 2020

wesleywiser left a comment

Choose a reason for hiding this comment

oli-obk commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 28, 2020

oli-obk commented Jul 28, 2020

bors commented Jul 28, 2020

bors commented Jul 29, 2020

oli-obk commented Jul 29, 2020

xldenis Jul 31, 2020

Choose a reason for hiding this comment

wesleywiser commented Aug 7, 2020

bors commented Aug 7, 2020

bors commented Aug 7, 2020

bors commented Aug 7, 2020