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.

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

Simplify switch sources #136959

Merged
merged 5 commits into from
Feb 18, 2025
Merged

Simplify switch sources #136959

merged 5 commits into from
Feb 18, 2025

Conversation

nnethercote
Copy link
Contributor

SwitchSources and the code around it can be simplified.

r? @tmiasko

@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 Feb 13, 2025
@nnethercote
Copy link
Contributor Author

As per the "useful comment" in the second last commit, SwitchSources is never actually instantiated right now, but I figure it's still good to simplify code and improve documentation, if only so some future person doesn't spend time working this stuff out the way I did.

@tmiasko
Copy link
Contributor

tmiasko commented Feb 13, 2025

SwitchSources is a hashmap containing SmallVec<[Option<u128>; 1]> elements. Each SmallVec has a predictable form: N Some elements followed by a single None element.

Elements from a particular SmallVec describe switch values with the same target block. The None element will be present only in a SmallVec describing a target of an otherwise branch.

@nnethercote nnethercote force-pushed the simplify-SwitchSources branch from c2720aa to d53670f Compare February 14, 2025 03:38
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2025

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

@nnethercote
Copy link
Contributor Author

The None element will be present only in a SmallVec describing a target of an otherwise branch.

Hmm, good point. I misunderstood that part.

I have removed some commits and added some new ones. This code is a bit tricky and I think it's good to make it simpler and clearer.

@bors
Copy link
Contributor

bors commented Feb 14, 2025

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

@nnethercote
Refactor apply_effects_in_block.
1300310 
Very minor changes that will make the next few commits easier to follow.
@nnethercote
Add a useful comment.
23dbff8
@nnethercote
Add SwitchTargetValue.
8403d39 
This is much clearer than `Option<u128>`.
@nnethercote
Update and clarify the comment on SwitchTargets.
db1ca60
@nnethercote
Remove SwitchIntTarget.
3b81d9d 
It's only passed to `Analysis::apply_switch_int_edge_effect`, and the
existing impls of that method only use the `value` field. So pass that
instead.
@nnethercote nnethercote force-pushed the simplify-SwitchSources branch from d53670f to 3b81d9d Compare February 16, 2025 23:04
@nnethercote
Copy link
Contributor Author

I have addressed the comments, so this code is ready for final review, thanks.

@tmiasko
Copy link
Contributor

tmiasko commented Feb 17, 2025

@bors r+

@bors
Copy link
Contributor

bors commented Feb 17, 2025

📌 Commit 3b81d9d has been approved by tmiasko

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 Feb 17, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 17, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2025
@bors
Auto merge of rust-lang#137176 - matthiaskrgr:rollup-eht05gr, r=matth…
de91711 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#136959 (Simplify switch sources)
 - rust-lang#137020 (Pass vendored sources from bootstrap to generate-copyright)
 - rust-lang#137073 (boostrap: skip no_std targets in Std doc step)
 - rust-lang#137165 (Use `tell` for `<File as Seek>::stream_position`)
 - rust-lang#137166 (Update default loongarch code model in docs)
 - rust-lang#137168 (correct comment)
 - rust-lang#137169 (CI: rfl: move job forward to Linux v6.14-rc3)
 - rust-lang#137170 (Allow configuring jemalloc per target)
 - rust-lang#137173 (Subtree update of `rust-analyzer`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 005de38 into rust-lang:master Feb 18, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 18, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2025
@rust-timer
Unrolled build for rust-lang#136959
18a3746 
Rollup merge of rust-lang#136959 - nnethercote:simplify-SwitchSources, r=tmiasko

Simplify switch sources

`SwitchSources` and the code around it can be simplified.

r? `@tmiasko`
@nnethercote nnethercote deleted the simplify-SwitchSources branch February 18, 2025 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@tmiasko tmiasko tmiasko left review comments

Assignees

@tmiasko tmiasko

Labels
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
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nnethercote @tmiasko @rustbot @bors @RalfJung