Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mir-opt] SimplifyLocals should also clean up debuginfo #110702

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

scottmcm
Copy link
Member

When not asking for debuginfo or when only asking for line-info, there's no need to treat a Local as live just because it's referenced in a piece of debug info.

This is particularly true for inlined functions -- today https://rust.godbolt.org/z/KqsrjfGja we keep around a bunch of useless things like debug info for the parameter to wrapping_neg inside wrapping_sub inside wrapping_byte_sub inside post_inc_start inside slice::Iter::next, even though we never actually use that local because its reads/writes got optimized away.

That meant that a bunch of mir-opt tests had everything interesting optimize away with this change, so there are a bunch of changes in here to properly mark them as unit tests or to pass -C debuginfo=2 as a way to suppress removing a bunch of this dead code.

@rustbot
Copy link
Collaborator

rustbot commented Apr 22, 2023

r? @cjgillot

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

@rustbot 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 Apr 22, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 22, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@@ -5,8 +5,7 @@ fn unwrap_unchecked(_1: Option<T>) -> T {
let mut _0: T; // return place in scope 0 at $DIR/unwrap_unchecked.rs:+0:54: +0:55
scope 1 (inlined #[track_caller] Option::<T>::unwrap_unchecked) { // at $DIR/unwrap_unchecked.rs:10:9: 10:27
debug self => _1; // in scope 1 at $SRC_DIR/core/src/option.rs:LL:COL
let mut _2: &std::option::Option<T>; // in scope 1 at $SRC_DIR/core/src/option.rs:LL:COL
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is a nice demonstration: the function had a stray &Option that got inlined from unwrap_unchecked that was never calculated, but still had a StorageLive+StorageDead because it was the self to Option::is_some.

Now it's removed as irrelevant, as seems entirely reasonable to me.

@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member Author

libcore.rlib: 0.3% smaller (56,292,736 → 56,090,538)
liballoc.rlib: 1.7% smaller (7,029,814 → 6,909,206)

@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor

I love the idea of removing useless cruft from MIR, but I fear touching debuginfo could backfire.

I we were to land this PR, we'll probably get a another report akin to #103655.
We currently have 2 PRs to fix that issue with DeadStoreElimination, blocked on a decision on the behaviour of mir-opt levels.

The harder part is that debuginfo is not crate-local: we may need to preserve more debuginfo in MIR in case the downstream crate doing the codegen needs it.

Removing storage statements on never-borrowed locals in RemoveStorageMarkers is probably the low-hanging fruit here.

@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch from e1965c1 to 8d09102 Compare April 22, 2023 22:14
@scottmcm
Copy link
Member Author

@cjgillot Do we really need to preserve per-variable debug info MIR no matter how the current crate is being compiled? That seems unfortunate. Why isn't it ok to say that if you want to debug into a dependency's variables, it needs to be compiled with debuginfo ≥ 2? After all, this change isn't removing variable debug info -- even when it's strictly useless -- if the session is asking for per-variable debug info.

If that's not ok, what would you think of a middle-ground: Today writes to a local never count as a use, but I could instead change it so that writes to a local count as a use iff that local is in debuginfo. That would allow removing the ones that are entirely useless -- there's no point in having debuginfo point at a local if that local is only ever undef -- but would keep from optimizing away writes to unused-except-for-debuginfo locals.

cc @wesleywiser, as the author of the linked bug report.

@Noratrieb
Copy link
Member

The harder part is that debuginfo is not crate-local: we may need to preserve more debuginfo in MIR in case the downstream crate doing the codegen needs it.

I don't think this makes sense. Semantically, monomorphizations are still part of the upstream crate and should follow its opts. If I compile regex without debuginfo because I want maximal performance but do enable debuginfo for my own code, I would not expect my regex instances to get debuginfo (worsening their performance).

@bors

This comment was marked as resolved.

@JakobDegen
Copy link
Contributor

Yeah, my one reaction here is that while I agree that respecting the downstream debuginfo is silly, if we are going to stop doing that we should at least do the correct thing in the other direction - ie if we compile a dep with -Cdebuginfo=2, that's the amount of debuginfo it should get, even if it's codegen'd downstream. I don't know how hard to implement this is.

@cjgillot
Copy link
Contributor

I convinced myself that this PR, as written, is the simpler and more consistent solution. Since there hasn't been a clear statement of policy on the matter of opt-level/debuginfo interaction, this deserves a FCP.

Change summary

This PR starts using the -Cdebuginfo flag to allow MIR optimizations to degrade debuginfo in MIR. This is a change from the current policy in which MIR opts preserve debuginfo at the expense of optimizations. I believe it consistent with the discussion in https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bsteering.5D.202023-03-17.20scope.20and.20goals.20of.20rust-lang.2Frust.20opti in which using two flags instead of one (opt-level and debuginfo) was discussed.

In the current behaviour, MIR always have full debug info whatever -Cdebuginfo says, and the crate doing codegen decides what is the amount to give to LLVM.

In the proposed behaviour, MIR will respect the debuginfo level of the crate that MIR-optimizes it. This means that generics from a dependency compiled with -Cdebuginfo=0 will have degraded debuginfo, whatever the flags for the downstream crates are. The downstream crate doing codegen will still decide what is gives to LLVM, but may have less to start with.

This change is user visible, although the behaviour of -Cdebuginfo on MIR is currently undocumented. The impact is however unlikely, except:

  • manually invoked rustc with different debuginfo levels;
  • rlib files shipped by a vendor with low debuginfo level.

This creates many different combinations of debuginfo, weird cases with MIR inlining. However, I think this is the right approach: if the user requests a specific debuginfo level on one crate, possibly different from the one of other crates, we should respect this request, and leverage it into optimizations.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 29, 2023

Team member @cjgillot has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 29, 2023
@JakobDegen
Copy link
Contributor

I think it's worth mentioning that the opt level policy stuff which I've been discussing was not meant to cover this case - I agree that generic functions not obeying the debug info level of the crate they are defined in is basically just a bug.

@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch from 5e314a2 to f69f547 Compare April 30, 2023 08:08
@bors

This comment was marked as resolved.

@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch 2 times, most recently from ee43236 to 0d86513 Compare May 4, 2023 17:15
@@ -1,11 +0,0 @@
//@ known-bug: #101962
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#101962 was closed because misuse of an intrinsic is not a rustc bug (rust-lang/compiler-team#620), so I'm deleting this instead of preserving the ICE.

@@ -581,3 +627,20 @@ impl<'tcx> MutVisitor<'tcx> for LocalUpdater<'tcx> {
*l = self.map[*l].unwrap();
}
}

fn preserve_debug_even_if_never_generated(opts: &Options) -> bool {
if let Some(p) = opts.unstable_opts.inline_mir_preserve_debug {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

annot: this is the material change from when I first tried to do this.

The problems from before were that config.toml affected the standard library build, making the tests inconsistent, but obeying this -Z flag fixes those problems.

@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch 2 times, most recently from e04f801 to 7c7e13a Compare July 4, 2024 20:52
@scottmcm
Copy link
Member Author

scottmcm commented Jul 4, 2024

@rustbot ready

The actual mir-opt code now is nearly identical to when this had an r+ in #110702 (comment), but since that's over a year ago so I don't think I should just r= it.

I still think the core thesis here makes sense:

if the session doesn't ask for variable-level debug information, then SimplifyLocals shouldn't count a use in a VarDebugInfo as a use when removing unused locals.

If the local is used for other reasons then this doesn't do anything different from before.

That said, after #123949 this is somewhat less impactful than it was before.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2024
@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch from 7c7e13a to 294efde Compare July 4, 2024 21:01
@bors
Copy link
Contributor

bors commented Jul 13, 2024

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

@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch from 294efde to 3377309 Compare July 13, 2024 01:54
@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch from 3377309 to 83744cb Compare July 13, 2024 01:56
}

// For now we only remove basic debuginfo, like `foo => _3`, and don't attempt
// to clean up more complicated things like `foo => Foo { .0 => _2, .1 => _4 }`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not foo => _3.projection?
Why not composite fields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it to handle projections too, so long as there's only one local mentioned.

Added some comments to clarify why it doesn't accept composites. Basically I don't want to start tracking enough state to be able to handle that, at least in this PR.

@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the also-remove-unused-storage-markers branch from 4dfcb7d to 1c585a5 Compare July 13, 2024 19:15

let mut finder = SingleLocalFinder(Ok(None));
finder.visit_var_debug_info(info);
if let Ok(Some(local)) = finder.0 { Some(local) } else { None }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Index projections are forbidden in debuginfo, so this can be simplified.

if !self.preserve_debug && debug_info_is_for_simple_local(var_debug_info).is_some() {
// We don't want to have to track *conditional* uses (such as
// "`_4` is used iff `_5` is used" from `debug x => Foo(_4, _5)`),
// so if this mentions multiple locals we just treat them all as used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just drop parts of the composite? If _4 is unused, go from Foo { .0 => _4, .1 => _5 } to just Foo { .1 => _5 }?
This line can definitely just be a if !self.preserve_debug, and complexity will go in remove_unused_definitions_helper.

@cjgillot cjgillot 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 Jul 19, 2024
@bors
Copy link
Contributor

bors commented Aug 19, 2024

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

@alex-semenyuk 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 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.