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

Compute information about function parameters on THIR #101086

Merged
merged 11 commits into from
Sep 13, 2022

Conversation

cjgillot
Copy link
Contributor

This avoids some manipulation of typeck results while building MIR.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 27, 2022
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2022
@bors
Copy link
Contributor

bors commented Aug 29, 2022

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

@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2022

Please bless the ui test changes so they are easier to review

@rustbot author

@rustbot rustbot 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 Aug 29, 2022
@cjgillot cjgillot mentioned this pull request Aug 29, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@@ -2,7 +2,7 @@ note: no external requirements
--> $DIR/escape-argument-callee.rs:26:38
|
LL | let mut closure = expect_sig(|p, y| *p = y);
| ^^^^^^
| ^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

This is odd, I would have assumed the changes to cause the opposite diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We replace def_span by span_with_body for closures, so span is longer except in specially addressed cases. As this is a debugging feature, I did not bother to keep the former behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ui output regression is fine in single line cases, but for multi line closures it's quite bad (and that makes up like 2/3rd of the ui changes. I don't think we should merge this as is. If mitigations can be deployed at diagnostic rendering time that would be fine with me. Since it's all about nll diagnostics, it could be possible to fix the span at borrowck time.

@cjgillot cjgillot 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 Aug 30, 2022
@bors
Copy link
Contributor

bors commented Aug 31, 2022

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

@cjgillot cjgillot force-pushed the thir-param branch 2 times, most recently from 5846ef9 to 4990819 Compare August 31, 2022 20:27
@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit 49908191456a14a025e06f10d651e52f8e499a78 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Sep 1, 2022
@bors
Copy link
Contributor

bors commented Sep 1, 2022

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

@bors 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 Sep 1, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Sep 1, 2022

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit b9e1806 has been approved by oli-obk

It is now in the queue for this repository.

@cjgillot
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Sep 13, 2022

📌 Commit ce9daa2 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Sep 13, 2022
@bors
Copy link
Contributor

bors commented Sep 13, 2022

⌛ Testing commit ce9daa2 with merge c84083b...

@bors
Copy link
Contributor

bors commented Sep 13, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c84083b to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c84083b): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
0.4% [0.2%, 0.5%] 10
Regressions ❌
(secondary)
0.6% [0.3%, 1.6%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.2%, 0.5%] 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.

mean1 range count2
Regressions ❌
(primary)
2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)
1.6% [0.8%, 2.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 3
All ❌✅ (primary) 2.6% [2.6%, 2.6%] 1

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.

mean1 range count2
Regressions ❌
(primary)
2.2% [1.5%, 3.0%] 2
Regressions ❌
(secondary)
4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)
-1.7% [-1.7%, -1.7%] 1
Improvements ✅
(secondary)
-4.2% [-4.2%, -4.2%] 1
All ❌✅ (primary) 0.9% [-1.7%, 3.0%] 3

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added the perf-regression Performance regression. label Sep 13, 2022
@pnkfelix
Copy link
Member

  • between 0.38% and 0.47% regression for stm32f4 in various incremental scenarios. Approximately 0.25% on serde,clap,image for incremental.
  • it seems plausible that this is just overhead that we have to pay, in terms of adding more state to the THIR, in exchange for simpler MIR construction code.
  • (its also possible that this is just noise, though the listed benchmarks are not the same as the other ones that have been exhibiting bimodality this last week.)
  • marking as triaged.

@rustbot label: perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 14, 2022
@nnethercote
Copy link
Contributor

I strongly suspect these changes are not noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants