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

Make sure to run all mir analyses in check mode #108730

Closed
wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 4, 2023

Only do so after checking for previous errors as otherwise the ui test suite diff is too large.

a step towards something like #107510

This was previously only done for consts since at the time, only consts needed to run checks after borrowck or drop elab. With the move of const prop lints before optimizations, we should've switched this on for all mir bodies.

r? @ghost

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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 4, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 4, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 4, 2023
@bors
Copy link
Contributor

bors commented Mar 4, 2023

⌛ Trying commit 16ea61bc20aae8ce55d264f4dfc5946d96f6d4bb with merge ef6fb91f1639ca8eb8441f20e006ae031ff3aa98...

@bors
Copy link
Contributor

bors commented Mar 4, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ef6fb91f1639ca8eb8441f20e006ae031ff3aa98): 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)
5.7% [0.3%, 16.8%] 75
Regressions ❌
(secondary)
3.5% [0.7%, 18.1%] 51
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.3%, -0.3%] 2
All ❌✅ (primary) 5.7% [0.3%, 16.8%] 75

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.4% [0.9%, 9.1%] 39
Regressions ❌
(secondary)
2.3% [1.5%, 4.1%] 11
Improvements ✅
(primary)
-2.7% [-4.5%, -0.8%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [-4.5%, 9.1%] 41

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)
6.0% [0.7%, 13.3%] 60
Regressions ❌
(secondary)
4.2% [0.9%, 19.0%] 40
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 6.0% [0.7%, 13.3%] 60

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 4, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 4, 2023

cc @tmandry this PR removes a large source of situations where check builds don't report anything but full builds do. It's mainly about reporting lints like unconditional_panic and overflowing shift rhs.

As expected, it is a major regression. This is not changing anything around when PMEs are reported, it's an entirely different issue.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2023

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Comment on lines -1 to -5
note: tracking was triggered
|
= note: created extern static allocation of SIZE bytes (alignment ALIGN bytes) with id 1
= note: (no span available)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit baffled why this disappears. The test is actually starting MIR interpretation, and there are no allocations beyond the one with id 1. So my guess is MIR interpretation doesn't create it, but const prop or some other analysis pass does and we just reuse the cached one.

Copy link
Member

Choose a reason for hiding this comment

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

Did AllocIds shift? The PR might evaluate more things earlier now so const-eval uses AllocIds that previously were still available to Miri.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh 🤦 I didn't go up far enough with the ids, because I started seeing other ids that I thought came later.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2023
…rieb

Add regression test for rust-lang#98444

cc rust-lang#108730 this will need to be changed to a `check-fail` test once it lands.

Fixes rust-lang#98444
@rust-log-analyzer

This comment has been minimized.

@tmandry
Copy link
Member

tmandry commented Mar 7, 2023

Thanks for tagging me! To check my understanding, this is about fixing some (maybe all?) cases of optimization-dependent errors, and the affected errors we're seeing in tests happen to specifically be lints (rather than hard errors)?

This is not changing anything around when PMEs are reported, it's an entirely different issue.

I'm not 100% sure what you include when you say PMEs. Can I propose to use the terminology I wrote at the top of this doc: Non-local errors in Rust? In those terms, I think what you're talking about is probably a combination of parameter-dependent errors and use-dependent errors.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 7, 2023

This PR does not affect non-local errors. It just makes sure that at least all lints that operate just on function-local information get emitted. It is a prerequisite to reporting all non-local errors in check mode, as most of these lints will start getting reported if we start reporting non-local errors in check mode.

I wanted to open this PR separately so that we can observe the performance changes and diagnostics changes on their own instead of mixing them with the changes that would occur if we reported non-local errors in check mode.

@tmandry
Copy link
Member

tmandry commented Mar 8, 2023

Thanks, that makes sense.

So these lints previously depended on check vs build, anything else? They weren't dependent on whether a function was used or not?

(By the way, I don't feel that strongly about using the term "non-local errors", I care more about splitting out into sub-categories like "CLI-dependent" and "parameter-dependent". If these distinctions don't quite map onto the problem space from your point of view, that would be good to know, but my hope is that everything fits into one or more of those sub-categories.)

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 8, 2023

They weren't dependent on whether a function was used or not?

No, these lints are reported for dead code, too. Previously they were only reported for all const items and functions (even those in dead code)

@bors
Copy link
Contributor

bors commented Mar 13, 2023

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

@tmandry
Copy link
Member

tmandry commented Mar 21, 2023

The lang team discussed this today in our meeting (notes). We agreed that we consider this a bug fix and think it should be merged.

After that, we'd like to know how costly it is to go further, eliminating any errors that appear in check but not build.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 22, 2023

r? compiler

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 24, 2023

This is not changing anything around when PMEs are reported, it's an entirely different issue.
I am confused by this statement, given that the PR claims to fix #99682 which very much looks like a PME to me? It's not just about "lints like unconditional_panic", at least.

Whoops, wrong issue. i'll search for the right one

@RalfJung
Copy link
Member

Ah okay. So I'm trying to understand what exactly this diff does: some MIR checks like unsafety, or packed fields, were already run in check builds before. But ConstPropLint was not?

Looking at run_analysis_to_runtime_passes and the passes it calls, we are doing a lot of work here that is unnecessary. Ideally we'd run the lints but not all the optimizations. Sadly, Lint(const_prop_lint::ConstProp) runs after all the run_runtime_lowering_passes. Can this be moved up, effectively running the lint on analysis MIR and thus avoiding the need to generate runtime MIR for check builds?

tcx.ensure()
.mir_drops_elaborated_and_const_checked(ty::WithOptConstParam::unknown(def_id));
}
});
Copy link
Member

Choose a reason for hiding this comment

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

This looks pretty similar to the existing MIR_effect_checking above. Please add comments explaining why there are two of these blocks and what each of them is doing.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 24, 2023

So anyone using cargo check success to mean "this is guaranteed to compile" is already wrong.

yea... that's the question. Where do we draw the line. Right now the line is arbitrary. If we don't want to check more lints, we may want to consider relaxing a few other things in cargo check, too, just to speed things up.

@RalfJung
Copy link
Member

RalfJung commented Mar 24, 2023

I do think it would be valuable to get the const-prop-lints run in check mode. It's not what I would consider a PME, so not having them in check builds is quite surprising. Or put differently: I don't think we should have lints as late in the MIR pipeline as const-pro-lints currently is. We have analysis MIR and runtime MIR, and just conceptually it seems clear that lints should run on analysis MIR, no?

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 24, 2023

Looking at run_analysis_to_runtime_passes and the passes it calls, we are doing a lot of work here that is unnecessary. Ideally we'd run the lints but not all the optimizations. Sadly, Lint(const_prop_lint::ConstProp) runs after all the run_runtime_lowering_passes. Can this be moved up, effectively running the lint on analysis MIR and thus avoiding the need to generate runtime MIR for check builds?

Unfortunately this quickly causes cycle errors because the only query before that is mir_promoted, and I can't run it in that query, as const prop lint will try to compute layouts of things containing opaque types, which rely on borrowck having run, which relies on the result of mir_promoted. It could certainly be refactored to support this (const promotion works around that cycle after all), but it's nontrivial.

@RalfJung
Copy link
Member

Hm, I see. borrowck being needed for layout is surprising (layout shouldn't depend on lifetimes) but does make this tricky indeed.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 24, 2023

Hm, I see. borrowck being needed for layout is surprising (layout shouldn't depend on lifetimes) but does make this tricky indeed.

that's actually a very good point... We can grab the types from typeck for layouting, but we need type_of an opaque type to result in the type with correct lifetimes. I'll see if we can use this to eliminate all our layout cycle problems

@oli-obk oli-obk added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Mar 24, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 24, 2023

Marking this PR as blocked on moving the const prop lint earlier.

@bjorn3
Copy link
Member

bjorn3 commented Mar 24, 2023

borrowck being needed for layout is surprising (layout shouldn't depend on lifetimes)

One of the outputs of borrowck is the list of paths captured by a closure, which affects the layout of closures.

@compiler-errors compiler-errors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2023
@bors
Copy link
Contributor

bors commented Apr 3, 2023

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

@apiraino
Copy link
Contributor

hey 👋 what's the status for this PR? Still S-blocked? thanks for an update :)

cc @oli-obk

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 23, 2023

I tried moving const prop earlier, but that is a significant perf regression: #115756

I won't be able to work on this again before January.

@RalfJung
Copy link
Member

RalfJung commented Oct 24, 2023

We could run const-prop-lints after borrowck but still as part of analysis MIR -- or does that not help perf-wise? It could be the last pass in analysis MIR. But the point is, it shouldn't be in optimized_mir, as that query (for good reasons) is not fired in cargo check.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 3, 2024

We could run const-prop-lints after borrowck but still as part of analysis MIR

that's already what's happening. run_analysis_to_runtime_passes is run right after we ensure that borrowck has been run. But we don't necessarily run mir_drops_elaborated_and_const_checked (which runs borrowck and the other passes) in check mode (I mean, that is exactly what this PR does 😆 ).

One possibility would be to turn const prop lint into a query and only run that explicitly (and make it work on top of mir_promoted + mir_borrowck). I'll try that out.

@RalfJung
Copy link
Member

RalfJung commented Jan 3, 2024

Ah I see, I didn't realize we have all that stuff in run_runtime_lowering_passes that shouldn't be needed for check builds but runs before ConstPropLint.

Do we know why running ConstPropLint earlier is such a perf regression? Is it because MIR gets so much bigger? Would it help to switch the lint to a different analysis? It still does the full const-prop dance; we could possibly trade some lint power for performance here.

However I think also lost track of the big picture here. The critical PR for const blocks is #112879, right? This one here is "just" some nice-to-have better alignment of check builds and full builds in terms of which diagnostics are emitted?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 3, 2024

However I think also lost track of the big picture here.

same. I was sure this was a blocker, but I'm not even sure what the next one is

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 15, 2024
… r=<try>

Remove all ConstPropNonsense

We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via `OpTy` we get for said constants).

This helps moving the const prop lint out from the normal pipeline and running it just based on borrowck information. This in turn allows us to make progress on rust-lang#108730 (comment)

there are various follow up cleanups that can be done after this PR (e.g. not matching on Rvalue twice and doing binop checks twice), but lets try landing this one first.

r? `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2024
… r=cjgillot

Remove all ConstPropNonsense

We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via `OpTy` we get for said constants).

This helps moving the const prop lint out from the normal pipeline and running it just based on borrowck information. This in turn allows us to make progress on rust-lang#108730 (comment)

there are various follow up cleanups that can be done after this PR (e.g. not matching on Rvalue twice and doing binop checks twice), but lets try landing this one first.

r? `@RalfJung`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jan 26, 2024
Remove all ConstPropNonsense

We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via `OpTy` we get for said constants).

This helps moving the const prop lint out from the normal pipeline and running it just based on borrowck information. This in turn allows us to make progress on rust-lang/rust#108730 (comment)

there are various follow up cleanups that can be done after this PR (e.g. not matching on Rvalue twice and doing binop checks twice), but lets try landing this one first.

r? `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
Run const_prop_lint in check builds, too

implements rust-lang#108730 (comment)

Turns the const_prop_lint pass into a query that is run alongside borrowck.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2024
Run const_prop_lint in check builds, too

implements rust-lang#108730 (comment)

Turns the const_prop_lint pass into a query that is run alongside borrowck.
@Dylan-DPC
Copy link
Member

@oli-obk what's the status of this?

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 17, 2024

#120687 did not succeed. 1.4% average regression with up to 20% regressions

I don't see a way to make progress here

@oli-obk oli-obk closed this Apr 17, 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 perf-regression Performance regression. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.