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] simplify Repeats that don't actually repeat the operand #135322

Merged
merged 2 commits into from
Jan 12, 2025

Conversation

scottmcm
Copy link
Member

Created because when I was writing this case in GVN

https://github.com/rust-lang/rust/pull/133324/files#diff-292b215fdc6b59e3f3c4173c2270b14591c5673832fbfb05cd69a05c2ef0c30eR977-R979

I happened to notice that it worked for [x] but not for [x; 1], so figured it would be good to simplify that Repeat to the simpler Aggregate.

@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2025

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Jan 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the inst-simplify-repeat-one branch from b1a6483 to 699703e Compare January 10, 2025 07:09
@oli-obk oli-obk assigned oli-obk and unassigned Nadrieril Jan 10, 2025
// unreachable or can't be ZST
_ => false,
_ => Some(false),
Copy link
Contributor

Choose a reason for hiding this comment

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

exhaustively match here. This branch must not have false positives given that it means "definitely not"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Spurious false is benign in how it's actually used, but that's not what the comments say, so I should re-work it. Maybe just have the wildcard be _ => None, since that's more obviously fine, and add the known-false cases explicitly.

@scottmcm scottmcm 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 Jan 10, 2025
@bors
Copy link
Contributor

bors commented Jan 11, 2025

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

@scottmcm scottmcm force-pushed the inst-simplify-repeat-one branch from 7a55e8a to 7396ec3 Compare January 12, 2025 00:05
@scottmcm
Copy link
Member Author

@rustbot ready

@rustbot rustbot 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 Jan 12, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Jan 12, 2025

@bors r+

@bors
Copy link
Contributor

bors commented Jan 12, 2025

📌 Commit 7396ec3 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 Jan 12, 2025
@bors
Copy link
Contributor

bors commented Jan 12, 2025

⌛ Testing commit 7396ec3 with merge 13f3924...

@bors
Copy link
Contributor

bors commented Jan 12, 2025

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 12, 2025
@bors bors merged commit 13f3924 into rust-lang:master Jan 12, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 12, 2025
@scottmcm scottmcm deleted the inst-simplify-repeat-one branch January 12, 2025 09:09
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (13f3924): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.3%, secondary 7.8%)

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)
3.3% [3.3%, 3.3%] 1
Regressions ❌
(secondary)
7.8% [7.8%, 7.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.3% [3.3%, 3.3%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 761.42s -> 762.038s (0.08%)
Artifact size: 326.13 MiB -> 326.01 MiB (-0.04%)

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. 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.

8 participants