Take MIR dataflow analyses by mutable reference #108293

Jarcho · 2023-02-21T04:23:37Z

The main motivation here is any analysis requiring dynamically sized scratch memory to work. One concrete example would be pointer target tracking, where tracking the results of a dereference can result in multiple possible targets. This leads to processing multi-level dereferences requiring the ability to handle a changing number of potential targets per step. A (simplified) function for this would be fn apply_deref(potential_targets: &mut Vec<Target>) which would use the scratch space contained in the analysis to send arguments and receive the results.

The alternative to this would be to wrap everything in a RefCell, which is what MaybeRequiresStorage currently does. This comes with a small perf cost and loses the compiler's guarantee that we don't try to take multiple borrows at the same time.

For the implementation:

AnalysisResults is an unfortunate requirement to avoid an unconstrained type parameter error.
CloneAnalysis could just be Clone instead, but that would result in more work than is required to have multiple cursors over the same result set.
ResultsVisitor now takes the results type on in each function as there's no other way to have access to the analysis without cloning it. This could use an associated type rather than a type parameter, but the current approach makes it easier to not care about the type when it's not necessary.
MaybeRequiresStorage now no longer uses a RefCell, but the graphviz formatter now does. It could be removed, but that would require even more changes and doesn't really seem necessary.

rustbot · 2023-02-21T04:23:42Z

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-02-21T04:23:45Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

bors · 2023-02-25T10:06:10Z

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

eholk · 2023-03-27T19:56:25Z

Hey, sorry for the delay in reviewing this! It looks pretty good to me, but unfortunately we've gotten some merge conflicts. If you go ahead and rebase it I can r+ it for you.

apiraino · 2023-05-03T16:00:51Z

Switching to waiting on author to update this branch (sorry I only notice this now because the PR was still labelled as waiting on a review).

@Jarcho Feel free to request a review again with @rustbot ready, thanks!

@rustbot author

Jarcho · 2023-05-19T01:12:07Z

@rustbot ready

eholk · 2023-06-08T20:07:18Z

Thanks for the rebase, and sorry about my delay in reviewing it. This looks good!

@bors r+

bors · 2023-06-08T20:07:20Z

📌 Commit eaddc37 has been approved by eholk

It is now in the queue for this repository.

bors · 2023-06-08T23:58:48Z

⌛ Testing commit eaddc37 with merge 68c8fda...

bors · 2023-06-09T03:00:14Z

☀️ Test successful - checks-actions
Approved by: eholk
Pushing 68c8fda to master...

bors · 2023-06-09T03:00:14Z

☀️ Test successful - checks-actions
Approved by: eholk
Pushing 68c8fda to master...

rust-timer · 2023-06-09T05:07:11Z

Finished benchmarking commit (68c8fda): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.

	mean	range	count
Regressions ❌ (primary)	1.5%	[0.4%, 2.7%]	10
Regressions ❌ (secondary)	2.9%	[2.7%, 3.0%]	2
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[-1.3%, 2.7%]	11

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: 649.476s -> 649.387s (-0.01%)

Patches: * rust-lang/rust#112119 * rust-lang/rust#108293 (partially) * rust-lang/rust#112786 * rust-lang/rust#112060 * rust-lang/rust#112163 * rust-lang/rust#112238 * rust-lang/rust#112165

…r=oli-obk Export AnalysisResults trait in rustc_mir_dataflow Followup to rust-lang#108293 Re-exports the new trait defined in mentioned PR to make ResultsCursor::seek_before_primary_effect, ResultsCursor::seek_after_primary_effect... usable again outside the compiler itself.

reuses rust-lang/rust#113089 and the corresponding nightly

Take `&mut Results` in `ResultsVisitor` This fixes a small oversight from rust-lang#108293.

rustbot assigned eholk Feb 21, 2023

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 21, 2023

This comment has been minimized.

Sign in to view

Jarcho force-pushed the mut_analyses branch 2 times, most recently from 75e7dc9 to cdca6e2 Compare February 21, 2023 05:11

This comment has been minimized.

Sign in to view

Jarcho force-pushed the mut_analyses branch from cdca6e2 to 13ce52e Compare February 21, 2023 05:40

Jarcho mentioned this pull request Mar 5, 2023

Take MIR Analysis by &mut rust-lang/compiler-team#598

Closed

3 tasks

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 May 3, 2023

Jarcho force-pushed the mut_analyses branch 2 times, most recently from 9f519e6 to 5785cda Compare May 18, 2023 20:22

This comment has been minimized.

Sign in to view

Take MIR dataflow analyses by mutable reference.

eaddc37

Jarcho force-pushed the mut_analyses branch from 5785cda to eaddc37 Compare May 18, 2023 21:46

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 May 19, 2023

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 Jun 8, 2023

bors added merged-by-bors This PR was explicitly merged by bors. labels Jun 9, 2023

bors merged commit 68c8fda into rust-lang:master Jun 9, 2023

rustbot added this to the 1.72.0 milestone Jun 9, 2023

This was referenced Jun 26, 2023

Update to toolchain 2023-06-29 creusot-rs/creusot#819

Merged

Export AnalysisResults trait in rustc_mir_dataflow #113089

Merged

rust-cloud-vms bot pushed a commit to b-naber/rust that referenced this pull request Jun 29, 2023

make BorrowedLocalsResultsCursor compatible with rust-lang#108293

9fb6c16

floriangru added a commit to floriangru/creusot that referenced this pull request Jul 6, 2023

finished patching rust-lang/rust#108293

f468e4f

reuses rust-lang/rust#113089 and the corresponding nightly

Jarcho mentioned this pull request Sep 2, 2023

Take &mut Results in ResultsVisitor #115488

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 4, 2023

Rollup merge of rust-lang#115488 - Jarcho:mut_result_visitor, r=oli-obk

5adacdf

Take `&mut Results` in `ResultsVisitor` This fixes a small oversight from rust-lang#108293.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 5, 2023

Rollup merge of rust-lang#115488 - Jarcho:mut_result_visitor, r=oli-obk

e9d15ec

Take `&mut Results` in `ResultsVisitor` This fixes a small oversight from rust-lang#108293.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take MIR dataflow analyses by mutable reference #108293

Take MIR dataflow analyses by mutable reference #108293

Jarcho commented Feb 21, 2023 •

edited

Loading

rustbot commented Feb 21, 2023

rustbot commented Feb 21, 2023

This comment has been minimized.

This comment has been minimized.

bors commented Feb 25, 2023

eholk commented Mar 27, 2023

apiraino commented May 3, 2023

This comment has been minimized.

Jarcho commented May 19, 2023

eholk commented Jun 8, 2023

bors commented Jun 8, 2023

bors commented Jun 8, 2023

bors commented Jun 9, 2023

bors commented Jun 9, 2023

rust-timer commented Jun 9, 2023

Take MIR dataflow analyses by mutable reference #108293

Take MIR dataflow analyses by mutable reference #108293

Conversation

Jarcho commented Feb 21, 2023 • edited Loading

rustbot commented Feb 21, 2023

rustbot commented Feb 21, 2023

This comment has been minimized.

This comment has been minimized.

bors commented Feb 25, 2023

eholk commented Mar 27, 2023

apiraino commented May 3, 2023

This comment has been minimized.

Jarcho commented May 19, 2023

eholk commented Jun 8, 2023

bors commented Jun 8, 2023

bors commented Jun 8, 2023

bors commented Jun 9, 2023

bors commented Jun 9, 2023

rust-timer commented Jun 9, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Jarcho commented Feb 21, 2023 •

edited

Loading