Skip to content

Add codegen tests for additional cases where noop iterators get optimized away #95695

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 18, 2022
Merged

Add codegen tests for additional cases where noop iterators get optimized away #95695

merged 1 commit into from
Apr 18, 2022

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Apr 5, 2022

Optimizations have improved over time and now LLVM manages to optimize more in-place-collect noop-iterators to O(1) functions. This updates the codegen test to match.

Many but not all cases reported in #79308 work now.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(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 Apr 5, 2022
@the8472 the8472 force-pushed the vec-codegen-tests branch 2 times, most recently from d0d1e58 to 29db04d Compare April 5, 2022 19:43
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

📌 Commit 29db04dc46b23cc889e297c4edbf31cccad648f9 has been approved by Mark-Simulacrum

@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 Apr 11, 2022
@bors
Copy link
Collaborator

bors commented Apr 11, 2022

⌛ Testing commit 29db04dc46b23cc889e297c4edbf31cccad648f9 with merge 58d3248d6a416dedf5832f7bb30e2d0f7a408e31...

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 11, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472
Copy link
Member Author

the8472 commented Apr 13, 2022

A case of "works on my machine" 😔
I can reproduce the failure in by running in docker but not on my host system, even with a clean config.toml.

@the8472
Add codegen tests for additional cases where noop iterators get optim…
6935896 
…ized away

Optimization have improved over time and now LLVM manages to optimize more
in-place-collect noop-iterators to O(1) functions. This updates the codegen test to match.
@the8472 the8472 force-pushed the vec-codegen-tests branch from 29db04d to 6935896 Compare April 13, 2022 20:47
@the8472
Copy link
Member Author

the8472 commented Apr 13, 2022

@rustbot ready

It was the debug asserts being enabled in CI, I shouldn't have removed the ignore-debug line.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 16, 2022

📌 Commit 6935896 has been approved by Mark-Simulacrum

@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 Apr 16, 2022
@bors
Copy link
Collaborator

bors commented Apr 18, 2022

⌛ Testing commit 6935896 with merge 7b5408d...

@bors
Copy link
Collaborator

bors commented Apr 18, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 7b5408d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 18, 2022
@bors bors merged commit 7b5408d into rust-lang:master Apr 18, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 18, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7b5408d): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regression found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 1 0 0 1
mean2 1.0% 0.4% N/A N/A 1.0%
max 1.0% 0.4% N/A N/A 1.0%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@nikic nikic mentioned this pull request May 19, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
8 participants
@the8472 @rust-highfive @Mark-Simulacrum @bors @rust-log-analyzer @rust-timer @nikic @rustbot