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 SwitchInt handling #133328

Merged
merged 5 commits into from
Dec 19, 2024
Merged

Simplify SwitchInt handling #133328

merged 5 commits into from
Dec 19, 2024

Conversation

nnethercote
Copy link
Contributor

Dataflow handling of SwitchInt is currently complicated. This PR simplifies it.

r? @cjgillot

@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 Nov 22, 2024
@nnethercote
Copy link
Contributor Author

Details in the individual commits.

Let me know if I am sending you too many reviews, I can reassign if necessary.

It's worth noting my motivation for this change. I wanted to try making Borrowck a normal analysis whose results are computed with iterate_to_fixpoint instead of being cobbled together from the results of Borrows/MaybeUninitializedPlaces/EverInitializedPlaces. When I tried to do this the complexity of SwitchInt handling completely blocked me from doing it. After doing the simplification in this PR, it was not just possible, but fairly easy. In the end, making Borrowck a normal analysis turned out to not work well -- it slowed compile times noticeably -- but it was a good proof of concept that the new structure is simpler and more flexible.

@bors
Copy link
Contributor

bors commented Nov 27, 2024

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

@nnethercote nnethercote force-pushed the simplify-SwitchInt-handling branch from 4cc04f3 to 6e4ad2c Compare November 27, 2024 08:16
@nnethercote
Copy link
Contributor Author

I rebased.

@nnethercote
Copy link
Contributor Author

@cjgillot: two week review ping

@nnethercote nnethercote mentioned this pull request Dec 5, 2024
Merged
@nnethercote nnethercote force-pushed the simplify-SwitchInt-handling branch from 6e4ad2c to 8e59df9 Compare December 6, 2024 04:49
@nnethercote
Copy link
Contributor Author

This shouldn't affect perf, but let's check that:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 6, 2024
@bors
Copy link
Contributor

bors commented Dec 6, 2024

⌛ Trying commit 8e59df9 with merge 79ce057...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2024
@bors
Auto merge of rust-lang#133328 - nnethercote:simplify-SwitchInt-handl…
79ce057 
…ing, r=<try>

Simplify `SwitchInt` handling

Dataflow handling of `SwitchInt` is currently complicated. This PR simplifies it.

r? `@cjgillot`
@bors
Copy link
Contributor

bors commented Dec 6, 2024

☀️ Try build successful - checks-actions
Build commit: 79ce057 (79ce057068524c8b9f939ad5a1fad606720b88a1)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (79ce057): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 1.5%, secondary 2.4%)

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

Cycles

Results (secondary 3.2%)

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)		 - - 0
Regressions ❌
(secondary)		 3.2% [2.8%, 3.5%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 769.915s -> 767.459s (-0.32%)
Artifact size: 330.89 MiB -> 330.90 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 6, 2024
@bors
Copy link
Contributor

bors commented Dec 14, 2024

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

@nnethercote
Add a comment to MaybeInitializedPlaces::apply_terminator_effect.
848610f 
I tried reordering this method to more closely match
`MaybeUninitializedPlaces::apply_terminator_effect`, but doing so breaks
tests.
@nnethercote
Simplify dataflow SwitchInt handling.
4d8316f 
Current `SwitchInt` handling has complicated control flow.

- The dataflow engine calls `Analysis::apply_switch_int_edge_effects`,
  passing in an "applier" that impls `SwitchIntEdgeEffects`.
- `apply_switch_int_edge_effects` possibly calls `apply` on the applier,
  passing it a closure.
- The `apply` method calls the closure on each `SwitchInt` edge.
- The closure operates on the edge.

I.e. control flow goes from the engine, to the analysis, to the applier
(which came from the engine), to the closure (which came from the
analysis). It took me a while to work this out.

This commit changes to a simpler structure that maintains the important
characteristics.

- The dataflow engine calls `Analysis::get_switch_int_data`.
- `get_switch_int_data` returns an `Option<Self::SwitchIntData>` value.
- If that returned value was `Some`, the dataflow engine calls
  `Analysis::apply_switch_int_edge_effect` on each edge, passing the
  `Self::SwitchIntData` value.
- `Analysis::apply_switch_int_edge_effect` operates on the edge.

I.e. control flow goes from the engine, to the analysis, to the
engine, to the analysis.

Added:
- The `Analysis::SwitchIntData` assoc type and the
  `Analysis::get_switch_int_data` method. Both only need to be
  defined by analyses that look at `SwitchInt` terminators.
- The `MaybePlacesSwitchIntData` struct, which has three fields.

Changes:
- `Analysis::apply_switch_int_edge_effects` becomes
  `Analysis::apply_switch_int_edge_effect`, which is a little simpler
  because it's dealing with a single edge instead of all edges.

Removed:
- The `SwitchIntEdgeEffects` trait, and its two impls:
  `BackwardSwitchIntEdgeEffectsApplier` (which has six fields) and
  `ForwardSwitchIntEdgeEffectsApplier` structs (which has four fields).
- The closure.

The new structure is more concise and simpler.
@nnethercote
Factor out some shared code from Maybe{In,Unin}itializedPlaces.
c8c50f4
@nnethercote
Remove opt_clone_from_or_clone.
83f3d20 
Switches to the idiom used elsewhere of calling `Analysis::bottom_value`
to initialize a `state` value outside a loop, and then using
`clone_from` to update it within the loop. This is simpler and has no
impact on performance.
@nnethercote
Remove a FIXME comment.
5f40942 
It is possible to avoid the clone as suggested in the comment. It would
require introducing an enum with two variants
`CloneBeforeModifying(&Domain)` and `Modifiable(&mut Domain)`. But it's
not worth the effort, because this code path just isn't very hot. E.g.
when compiling a large benchmark like `cargo-0.60.0` it's only hit a few
thousand times.
@nnethercote nnethercote force-pushed the simplify-SwitchInt-handling branch from 9da7871 to 5f40942 Compare December 15, 2024 22:53
@nnethercote
Copy link
Contributor Author

Let's try a different reviewer: r? @tmiasko

Best reviewed one commit at a time. See this comment for the specific motivation beyond "it makes thing simpler".

@rustbot rustbot assigned tmiasko and unassigned cjgillot Dec 17, 2024
@tmiasko
Copy link
Contributor

tmiasko commented Dec 18, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Dec 18, 2024

📌 Commit 5f40942 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 Dec 18, 2024
@bors
Copy link
Contributor

bors commented Dec 18, 2024

⌛ Testing commit 5f40942 with merge c434b4b...

@bors
Copy link
Contributor

bors commented Dec 19, 2024

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing c434b4b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2024
@bors bors merged commit c434b4b into rust-lang:master Dec 19, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 19, 2024
@nnethercote nnethercote deleted the simplify-SwitchInt-handling branch December 19, 2024 03:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c434b4b): 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 4.1%, secondary 3.5%)

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)		 4.1% [0.9%, 7.3%] 2
Regressions ❌
(secondary)		 3.5% [3.5%, 3.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 4.1% [0.9%, 7.3%] 2

Cycles

Results (secondary -2.3%)

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

Binary size

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

Bootstrap: 771.62s -> 770.535s (-0.14%)
Artifact size: 330.35 MiB -> 330.36 MiB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tmiasko tmiasko

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
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @bors @rust-timer @tmiasko @cjgillot @rustbot