-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
large_assignments: Lint on specific large args passed to functions #116520
Conversation
This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras This PR changes Stable MIR cc @oli-obk, @celinval, @spastorino Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only reviewed the first commit. I don't understand why you needed to change HIR types to be !Copy. Is that a leftover from previous experimentation adding such a Vec to the HIR?
b4384b9
to
1f1bc69
Compare
748ed9c
to
f43309d
Compare
This comment has been minimized.
This comment has been minimized.
f43309d
to
bbd139c
Compare
This comment has been minimized.
This comment has been minimized.
bbd139c
to
97f5bd1
Compare
This comment has been minimized.
This comment has been minimized.
Looks like I need to bless 309 mir-opt tests to make CI pass. To not make the diff too noisy, would you mind doing a code review of the first commit before I bless tests? Am I on the right track? @rustbot ready |
97f5bd1
to
b642da8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did write a review last night but seem to have fallen asleep while doing so.
None of this is important or even nits you must address, pick what you like
I like how it works now
This comment has been minimized.
This comment has been minimized.
Ah, I think we shouldn't bless the mir opt tests. Instead change the mir printing logic to skip the Spanned and directly print the Operand |
b642da8
to
058e557
Compare
@RalfJung Do you think this (
to
? I would be happy to do that later if you want it done. |
Done! |
The change in compiler/rustc_middle/src/mir/syntax.rs LGTM, assuming this doesn't cost too much perf. We should do a perf run before landing. Let me know when you're ready for that, I can tell the bots to do it.
No strong opinion, but sure, why not. :) |
To enable improved accuracy of diagnostics in upcoming commits.
c15236b
to
8f440f0
Compare
large_assignments: Lint on specific large args passed to functions Requires lowering function call arg spans down to MIR, which is done in the second commit. Part of rust-lang#83518 Also see * https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/arg.20Spans.20for.20TerminatorKind.3A.3ACall.3F * https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/move_size_limit.20lint r? `@oli-obk` (E-mentor)
💥 Test timed out |
@bors retry apple timeout |
☀️ Test successful - checks-actions |
Finished benchmarking commit (92f2e0a): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 663.55s -> 663.149s (-0.06%) |
This regression is expected. We are tracking more spans in order to be able to produce correct diagnostics. We expect to be using this information more now that we have it. I do expect some improvements to be possible in borrowck errors and const prop lints @rustbot label: +perf-regression-triaged |
Fixes were done to address the following upstream changes: - rust-lang/rust#119606 - rust-lang/rust#119751 - rust-lang/rust#120025 - rust-lang/rust#116520 Resolves #2971 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
Make `TerminatorKind::Call::func` `Spanned` and remove `::fn_span` To get structural symmetry between `TerminatorKind::Call::func` and `TerminatorKind::Call::args`, make `func` `Spanned`, since `args` already is `Spanned`. This way we can remove the separate `fn_span` field. Doing this was briefly discussed [here](rust-lang#116520 (comment)). For easier review, this PR is split into two commits. The first commit is the actual change. The second commit just makes the code build again and is very repetitive. Can someone start a perf run of this change please?
Requires lowering function call arg spans down to MIR, which is done in the second commit.
Part of #83518
Also see
r? @oli-obk (E-mentor)