Only preserve DebugInfo in DeadStoreElimination if requested. #106852

cjgillot · 2023-01-14T12:11:02Z

The current pass may delete assignments to locals that appear in debuginfo, irrespective of the opt-level. This PR attempts to preserve such assignments.

Fixes #103655

rustbot · 2023-01-14T12:11:06Z

Failed to set assignee to JakobDegen: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2023-01-14T12:11:09Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

cjgillot · 2023-01-14T12:59:06Z

r? wg-mir-opt

cjgillot · 2023-01-14T13:15:30Z

@bors try @rust-timer queue

bors · 2023-01-14T13:15:39Z

⌛ Trying commit 7e940fd36a93faafc4f402b0423816942a65fc8c with merge 7dc1f7ab3906a64a8ed85774edd3b3805670f534...

bors · 2023-01-14T15:38:58Z

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

rust-timer · 2023-01-14T17:48:39Z

Finished benchmarking commit (7dc1f7ab3906a64a8ed85774edd3b3805670f534): 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.8%	[0.4%, 1.3%]	3
Regressions ❌ (secondary)	0.5%	[0.3%, 0.7%]	8
Improvements ✅ (primary)	-0.5%	[-0.8%, -0.3%]	7
Improvements ✅ (secondary)	-0.9%	[-1.4%, -0.4%]	4
All ❌✅ (primary)	-0.1%	[-0.8%, 1.3%]	10

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)	1.8%	[0.2%, 3.5%]	4
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-3.3%	[-3.8%, -2.8%]	2
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	0.1%	[-3.8%, 3.5%]	6

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

JakobDegen · 2023-01-15T01:25:38Z

compiler/rustc_mir_dataflow/src/framework/engine.rs

+            analysis.initialize_start_block(body, &mut entry_sets[mir::START_BLOCK]);
+        } else {
+            // For backward analyses, initialize blocks that exit MIR.
+            analysis.initialize_start_block(body, &mut init_value);


Using initialize_start_block for this feels weird. Can you instead treat locals with debuginfo as being read on return/resume terminators?

JakobDegen · 2023-01-15T01:26:53Z

Treating locals as being live when the function returns feels like a bit of a weird solution. It will still cause the dead write to be removed in an example like this:

x = 5;
x = 10;
dbg!(x);

See also #103657 for a different approach.

As DSE has soundness implications on DestProp:

It does not anymore. If I accidentally left a comment in saying otherwise, feel free to remove it.

cjgillot · 2023-01-15T10:10:36Z

Treating locals as being live when the function returns feels like a bit of a weird solution. It will still cause the dead write to be removed in an example like this:
x = 5;
x = 10;
dbg!(x);

That's a bit of the point. My proposal is to remove all the assignments that are "overwritten" without being read, but stop just before removing all assignments entirely. Effectively, this amounts to only keep the last assignment.

The debugger won't see the dead assignments, but will see the final value, and the variable will still exist. I'm tempted to consider this a middle ground between optimization and debuggability.

After all, debugging an optimizing build, one's bound to see pink elephants 😄

JakobDegen · 2023-01-16T23:07:38Z

Hmm, ok, makes sense. Tbh, I think we should also block this on the same discussion as on the other PR. Once we have the T-compiler design meeting on this topic, we can write up an updated policy on the opt levels and make a decision at that point. Feels more principled than merging either of these PRs before that

davidtwco · 2023-01-25T13:42:38Z

r? @JakobDegen

bors · 2023-02-06T20:17:51Z

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

saethlin · 2024-05-26T20:25:50Z

(I'm going to see if I can finish up this PR)

I pushed a rebase because it produced a new test failure, which I've fixed. At least the first part of the test suite looks good locally now? Let's see...

saethlin · 2024-05-26T21:27:00Z

@cjgillot This now passes CI. But I think you had some lingering concern about some sort of inconsistency? Up above, you said:

There are differences in the blessed output depending on the target. I haven't tracked where this comes from, probably a change in debuginfo flags somewhere.

But all the mir-opt test output looks good to me; is there some other kind of experiment or test you were doing locally?

bors · 2024-06-03T13:33:03Z

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

bors · 2024-06-11T04:18:44Z

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

bors · 2024-06-17T21:35:57Z

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

scottmcm · 2024-06-20T16:06:39Z

tests/mir-opt/pre-codegen/optimizes_into_variable.main.PreCodegen.after.32bit.panic-abort.mir

            scope 3 {
-                debug z => const 42_u32;
+                debug z => _3;


This seems strange? Feels like either we should remove the debuginfo for this variable or it should have the constant. Having debuginfo pointing to something that's never initialized feels weird at PreCodegen phase.

(It'd be fine as an intermediate thing, to be removed in a subsequent pass, but seems weird like this.)

Should we maybe DeadStoreElem after SingleUseConsts?

Sure, I pushed a commit that changes the pass order. I'm not sure it's always better, but it fixes this one problem.

bors · 2024-06-29T07:56:08Z

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

cjgillot · 2024-07-13T12:42:51Z

compiler/rustc_mir_transform/src/lib.rs

            &gvn::GVN,
            &simplify::SimplifyLocals::AfterGVN,
            &dataflow_const_prop::DataflowConstProp,
            &single_use_consts::SingleUseConsts,
+            &dead_store_elimination::DeadStoreElimination::Initial,


I'd rather see SingleUseConsts run multiple times than move DSE.
Having DSE run early is interesting to reduce work and create additional opportunities for GVN.
Furthermore, SingleUseConsts should be cheap enough to run multiple times, doesn't it?

SingleUseConsts is just O(statements), so should be fine to run multiple times.

Come to think of it, doing it after inlining and before InstSimplify probably makes sense, as otherwise I don't think that https://github.com/cjgillot/rust/blob/a299aa5df79dd8e5a1405b97ddd7b367b61eecc6/compiler/rustc_mir_transform/src/instsimplify.rs#L121 will realistically find anything to simplify.

🤔 I've tried to apply this concept, and it seems to have fixed a FIXME in the SingleUseConsts test?

scottmcm · 2024-07-14T01:41:56Z

tests/mir-opt/single_use_consts.if_const.SingleUseConsts.panic-unwind.diff

-         _1 = const <T as MyTrait>::ASSOC_BOOL;
-         switchInt(move _1) -> [0: bb2, otherwise: bb1];
-+         nop;
-+         switchInt(const <T as MyTrait>::ASSOC_BOOL) -> [0: bb2, otherwise: bb1];
+          nop;
+          switchInt(const <T as MyTrait>::ASSOC_BOOL) -> [0: bb2, otherwise: bb1];


fix: I think if you're going to run it multiple times you need to make it an enum and manually give each run a different name. Otherwise the tests have nothing to see any more, like here where the diff file is the diff from the second run where nothing happens because it's the first run that did it, and we do want the diff to actually show a diff in the MIR here.

scottmcm · 2024-07-14T01:42:53Z

tests/mir-opt/single_use_consts.if_const_debug.SingleUseConsts.panic-unwind.diff

-         debug my_bool => _1;
-+         debug my_bool => const <T as MyTrait>::ASSOC_BOOL;
+          debug my_bool => const <T as MyTrait>::ASSOC_BOOL;


Ditto here; this test should be showing a change in the debuginfo for the pass.

rust-log-analyzer · 2024-07-21T18:29:54Z

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

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

------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:3aacb9c90579defe09351ac5e8ee504359f8054da6326ff19038f1b7c90e3cb2aafe33685c6d9b76ee8d2ccbd187ca80c46ab5380485abdd8c0ce7d69cd8d8fd:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---

warning: static `C_STR` is never used
##[warning] --> alloc_input.rs:8:12
  |
8 |     static C_STR: &std::ffi::CStr = c"cstr";
  |
  = note: `#[warn(dead_code)]` on by default

warning: function `check_type_id` is never used

bors · 2024-07-25T18:53:30Z

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

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 14, 2023

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

JakobDegen reviewed Jan 15, 2023

View reviewed changes

cjgillot force-pushed the debug-dse branch from 80a1b7d to 36f034c Compare January 15, 2023 09:58

cjgillot force-pushed the debug-dse branch 2 times, most recently from 6951a9b to a17b591 Compare January 15, 2023 10:33

rustbot assigned JakobDegen and unassigned davidtwco Jan 25, 2023

cjgillot force-pushed the debug-dse branch from a17b591 to 877aeaa Compare February 4, 2023 12:10

cjgillot force-pushed the debug-dse branch from 877aeaa to 41a0a4b Compare February 11, 2023 09:38

wesleywiser added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Feb 16, 2023

This comment has been minimized.

Sign in to view

cjgillot force-pushed the debug-dse branch from 56e70b4 to 107bd38 Compare April 21, 2024 12:42

This comment has been minimized.

Sign in to view

saethlin force-pushed the debug-dse branch from 107bd38 to f384f12 Compare May 26, 2024 20:24

saethlin force-pushed the debug-dse branch from f384f12 to 517dfc0 Compare June 4, 2024 01:05

saethlin force-pushed the debug-dse branch from 517dfc0 to 1a0105a Compare June 13, 2024 02:38

saethlin force-pushed the debug-dse branch from 1a0105a to 61fee46 Compare June 19, 2024 20:39

scottmcm reviewed Jun 20, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

saethlin force-pushed the debug-dse branch from 32f7ffa to f64c307 Compare June 29, 2024 15:35

This comment has been minimized.

Sign in to view

cjgillot commented Jul 13, 2024

View reviewed changes

saethlin force-pushed the debug-dse branch from a377030 to 27fae62 Compare July 13, 2024 22:25

This comment has been minimized.

Sign in to view

scottmcm reviewed Jul 14, 2024

View reviewed changes

cjgillot and others added 2 commits July 21, 2024 13:04

Only account for debuginfo if the user requests it.

89d3f11

Run SingleUseConsts before DeadStoreElimination::Initial

19c27fe

saethlin force-pushed the debug-dse branch from 27fae62 to 19c27fe Compare July 21, 2024 18:01

cjgillot mentioned this pull request Aug 27, 2024

dse: Eliminate dead assignment statements when debuginfo is not set to full #129611

Draft

alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 17, 2024

Only preserve DebugInfo in DeadStoreElimination if requested. #106852

Are you sure you want to change the base?

Only preserve DebugInfo in DeadStoreElimination if requested. #106852

Conversation

cjgillot commented Jan 14, 2023 • edited Loading

rustbot commented Jan 14, 2023

rustbot commented Jan 14, 2023

cjgillot commented Jan 14, 2023

cjgillot commented Jan 14, 2023

This comment has been minimized.

bors commented Jan 14, 2023

bors commented Jan 14, 2023

This comment has been minimized.

rust-timer commented Jan 14, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Choose a reason for hiding this comment

JakobDegen commented Jan 15, 2023

cjgillot commented Jan 15, 2023

JakobDegen commented Jan 16, 2023

davidtwco commented Jan 25, 2023

bors commented Feb 6, 2023

This comment has been minimized.

This comment has been minimized.

saethlin commented May 26, 2024 • edited Loading

saethlin commented May 26, 2024

bors commented Jun 3, 2024

bors commented Jun 11, 2024

bors commented Jun 17, 2024

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.

bors commented Jun 29, 2024

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 has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-log-analyzer commented Jul 21, 2024

bors commented Jul 25, 2024

cjgillot commented Jan 14, 2023 •

edited

Loading

saethlin commented May 26, 2024 •

edited

Loading