Introduce new dataflow implementation for available locals, use in existing pass #78928

simonvandel · 2020-11-10T20:04:55Z

This PR introduces an availablity dataflow analysis for locals, and uses it to reimplement (hopefully soundly) an existing pass that removes unneeded derefs.
The availability analysis should be pretty generic so can be used in further passes where the question "can i freely use this local here?" arises.

It's my first contribution using the dataflow framework, so i'm curious how my implementation can be improved.

Availability analysis

A local is available at a given program point, if the value of the local can freely be used at the given program point.
Consider the following example:

_1 = 4;
_2 = &_1;
_3 = *_2

In the above example, _2 is available at the third statement, so the statement can be simplified to _3 = _1.
In general, an available local can be used freely on any path from the definition of _2 to statement s, if _2 and its value is guaranteed to not be changed on all paths.

In the following example _2 is not available in bb2, since we do not know if _2 = &5 is executed.

bb0 {
  _2 = &_1;
  switchInt(_1) -> [4: bb1, otherwise: bb2]
}

bb1 {
  _2 = &5;
}

bb2 {
  _3 = *_2
}

fixes #78368

and use it to implement unneeded deref mir-opt pass

rust-highfive · 2020-11-10T20:04:59Z

r? @petrochenkov

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

petrochenkov · 2020-11-10T22:24:38Z

@ecstatic-morse is no longer active, so r? @jonas-schievink or @tmiasko, I guess?

jonas-schievink · 2020-11-10T22:37:32Z

@bors try @rust-timer queue

rust-timer · 2020-11-10T22:37:34Z

Awaiting bors try build completion

bors · 2020-11-10T22:37:45Z

⌛ Trying commit 44437da with merge e1fd1a66ec83a1be47f918e7c546d28a13b90012...

bors · 2020-11-10T23:23:26Z

☀️ Try build successful - checks-actions
Build commit: e1fd1a66ec83a1be47f918e7c546d28a13b90012 (e1fd1a66ec83a1be47f918e7c546d28a13b90012)

rust-timer · 2020-11-10T23:23:28Z

Queued e1fd1a66ec83a1be47f918e7c546d28a13b90012 with parent cf9cf7c, future comparison URL.

rust-timer · 2020-11-11T01:01:34Z

Finished benchmarking try commit (e1fd1a66ec83a1be47f918e7c546d28a13b90012): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

jonas-schievink

Unfortunately I don't really have the capacity to do a full review here, so r? @oli-obk

jonas-schievink · 2020-11-11T01:26:08Z

src/test/mir-opt/instrument_coverage.main.InstrumentCoverage.diff

-          resume;                          // scope 0 at /the/src/instrument_coverage.rs:10:1: 16:2
-      }
-  }
-


Why was this deleted?

I renamed a test, but since bless does not cleanup already created diffs I ran a script myself to delete all diff files, and then ran bless again. Since ci passes, this file is not used in any test afaict.

jonas-schievink · 2020-11-11T02:19:30Z

compiler/rustc_mir/src/dataflow/impls/available_locals.rs

+/// In the above example, `_2` is available at the third statement, so the statement can be
+/// simplified to `_3 = _1`.
+/// In general, an available local can be used freely on any path from the definition of `_2` to
+/// statement `s`, if `_2` and its value is guaranteed to not be changed on all paths.


What's the difference between this and reaching definitions? It doesn't really seem like the best idea to introduce an ad-hoc analysis for a single minor optimization instead of using a well-known analysis that may have other uses in the future.

I may be a bit off on the terminology here. What I implemented sounds like reaching definitions, yeah. Did I miss an implementation already in rustc?

"available expression analysis" is often the term I see used in compiler theory, which refers to an analysis that can answer if an expression can be reused at a point since it is not modified along the way from its definition.

I named the pass I implemented "available locals" since it tracks locals, not expressions. But I could probably more precisely name it reaching definitions.

I agree that a new dataflow analysis solely for this one optimization is a bit overkill, but I think the analysis in general can be applied in a lot of coming or existing passes.

jonas-schievink · 2020-11-11T02:20:32Z

r? @oli-obk

simonvandel · 2020-11-11T21:31:06Z

compiler/rustc_mir/src/dataflow/impls/available_locals.rs

+        _args: &[mir::Operand<'tcx>],
+        _dest_place: mir::Place<'tcx>,
+    ) {
+        // Conservatively do not try to reason about calls


We should take care here to invalidate operands that move locals, i'll fix this in a commit

…ive so do it as late as possible

If we need them, they will be added in the visitor that goes through assigns

It actually had no gain on my benchmark, but LLVM did decide to inline it when it now can

oli-obk · 2020-11-13T08:44:26Z

compiler/rustc_mir/src/transform/unneeded_deref.rs

@@ -129,7 +131,7 @@ impl<'a, 'tcx> ResultsVisitor<'a, 'tcx> for UnneededDerefVisitor<'a, 'tcx> {
        stmt: &'mir Statement<'tcx>,
        location: Location,
    ) {
-        self.state = Some(state.clone());
+        self.state = state;


There's no need to use self for the visit_rvalue. You could also create a separate visitor that has a reference to Self as a field and thus can have a reference with a lifetime in the state field. If performance is the same, then I would definitely prefer that.

oli-obk · 2020-11-13T08:45:04Z

@bors try @rust-timer queue

rust-timer · 2020-11-13T08:45:05Z

Awaiting bors try build completion

bors · 2020-11-13T08:45:17Z

⌛ Trying commit 78b013c with merge 1baa642e3635e423d55a5980b5a83a02ed883bee...

bors · 2020-11-13T09:31:06Z

☀️ Try build successful - checks-actions
Build commit: 1baa642e3635e423d55a5980b5a83a02ed883bee (1baa642e3635e423d55a5980b5a83a02ed883bee)

rust-timer · 2020-11-13T09:31:08Z

Queued 1baa642e3635e423d55a5980b5a83a02ed883bee with parent a38f8fb, future comparison URL.

rust-timer · 2020-11-13T13:15:26Z

Finished benchmarking try commit (1baa642e3635e423d55a5980b5a83a02ed883bee): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

oli-obk · 2020-11-13T14:04:43Z

The regressions are now mostly gone, but so are the improvements

It's only important that the place referenced and the local we store it in is available at the time we try to apply the deref optimization

… visitor

simonvandel · 2020-11-15T19:08:40Z

I'm a bit about of ideas for further things that can be done to improve the performance of the analysis. Can i get a perf run for the latest changes?

bjorn3 · 2020-11-15T20:59:55Z

@bors try @rust-timer queue

rust-timer · 2020-11-15T20:59:56Z

Awaiting bors try build completion

bors · 2020-11-15T21:00:14Z

⌛ Trying commit 7102506 with merge 18df179800c8caf37dfb5354d01f8792a7b34d38...

bors · 2020-11-15T21:47:46Z

☀️ Try build successful - checks-actions
Build commit: 18df179800c8caf37dfb5354d01f8792a7b34d38 (18df179800c8caf37dfb5354d01f8792a7b34d38)

rust-timer · 2020-11-15T21:47:49Z

Queued 18df179800c8caf37dfb5354d01f8792a7b34d38 with parent 603ab5b, future comparison URL.

rust-timer · 2020-11-15T23:26:20Z

Finished benchmarking try commit (18df179800c8caf37dfb5354d01f8792a7b34d38): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

crlf0710 · 2020-12-18T11:31:49Z

@simonvandel Ping from triage: What's the current status of this? And it has merge conflicts now.

simonvandel · 2021-01-03T15:15:07Z

I'll close the PR. The current implementation has some performance problems which is not obvious to me how to resolve. If MIR ever becomes SSA that will greatly simplify the implementation. If MIR gets optimized better in the future, it might then make sense to revive the PR, so it has less code to churn through.

Introduce new dataflow implementation for available locals,

44437da

and use it to implement unneeded deref mir-opt pass

rust-highfive assigned petrochenkov Nov 10, 2020

rust-highfive added the S-waiting-on-review label Nov 10, 2020

rust-highfive assigned jonas-schievink and unassigned petrochenkov Nov 10, 2020

jonas-schievink reviewed Nov 11, 2020

View reviewed changes

rust-highfive assigned oli-obk and unassigned jonas-schievink Nov 11, 2020

simonvandel commented Nov 11, 2020

View reviewed changes

simonvandel added 7 commits November 12, 2020 19:59

separate opt/no-opt tests

c099099

Perf: avoid cloning state

ef1dd41

return pointer had wrong index

086b98f

perf: preparing a AvailableLocals analysis is currently pretty expens…

20e1973

…ive so do it as late as possible

perf: there is no need to add entries for vars and temps

191eb30

If we need them, they will be added in the visitor that goes through assigns

no need to call super_place

ee5f9d1

Mark Place::as_ref as inlineable

78b013c

It actually had no gain on my benchmark, but LLVM did decide to inline it when it now can

oli-obk reviewed Nov 13, 2020

View reviewed changes

simonvandel added 5 commits November 13, 2020 22:58

Skip going through participating locals

6cef14f

It's only important that the place referenced and the local we store it in is available at the time we try to apply the deref optimization

It is cheaper to manually find the assignments, rather than through a…

b19b5fa

… visitor

Only walk through what we need

f1e4b09

do not clone rvalue

54b95af

tidy

ed574a0

oli-obk mentioned this pull request Nov 14, 2020

StorageLive (and even StorageDead) may be unnecessary in MIR. #68622

Open

simonvandel added 4 commits November 15, 2020 18:20

set indices on the fly

a046437

skip adding locals with projections to map

cc5588b

remove unneeded invalidation

6cce57b

eliminate unsafe with a separate visitor

88cf9cc

Fix unwrap panic

7102506

JohnCSimon added S-waiting-on-author and removed S-waiting-on-review labels Nov 30, 2020

simonvandel closed this Jan 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new dataflow implementation for available locals, use in existing pass #78928

Introduce new dataflow implementation for available locals, use in existing pass #78928

simonvandel commented Nov 10, 2020 •

edited by camelid

Loading

rust-highfive commented Nov 10, 2020

petrochenkov commented Nov 10, 2020

jonas-schievink commented Nov 10, 2020

rust-timer commented Nov 10, 2020

bors commented Nov 10, 2020

bors commented Nov 10, 2020

rust-timer commented Nov 10, 2020

rust-timer commented Nov 11, 2020

jonas-schievink left a comment

jonas-schievink Nov 11, 2020

simonvandel Nov 11, 2020

jonas-schievink Nov 11, 2020

simonvandel Nov 11, 2020

jonas-schievink commented Nov 11, 2020

simonvandel Nov 11, 2020

oli-obk Nov 13, 2020

oli-obk commented Nov 13, 2020

rust-timer commented Nov 13, 2020

bors commented Nov 13, 2020

bors commented Nov 13, 2020

rust-timer commented Nov 13, 2020

rust-timer commented Nov 13, 2020

oli-obk commented Nov 13, 2020

simonvandel commented Nov 15, 2020

bjorn3 commented Nov 15, 2020

rust-timer commented Nov 15, 2020

bors commented Nov 15, 2020

bors commented Nov 15, 2020

rust-timer commented Nov 15, 2020

rust-timer commented Nov 15, 2020

crlf0710 commented Dec 18, 2020

simonvandel commented Jan 3, 2021

Introduce new dataflow implementation for available locals, use in existing pass #78928

Introduce new dataflow implementation for available locals, use in existing pass #78928

Conversation

simonvandel commented Nov 10, 2020 • edited by camelid Loading

rust-highfive commented Nov 10, 2020

petrochenkov commented Nov 10, 2020

jonas-schievink commented Nov 10, 2020

rust-timer commented Nov 10, 2020

bors commented Nov 10, 2020

bors commented Nov 10, 2020

rust-timer commented Nov 10, 2020

rust-timer commented Nov 11, 2020

jonas-schievink left a comment

Choose a reason for hiding this comment

jonas-schievink Nov 11, 2020

Choose a reason for hiding this comment

simonvandel Nov 11, 2020

Choose a reason for hiding this comment

jonas-schievink Nov 11, 2020

Choose a reason for hiding this comment

simonvandel Nov 11, 2020

Choose a reason for hiding this comment

jonas-schievink commented Nov 11, 2020

simonvandel Nov 11, 2020

Choose a reason for hiding this comment

oli-obk Nov 13, 2020

Choose a reason for hiding this comment

oli-obk commented Nov 13, 2020

rust-timer commented Nov 13, 2020

bors commented Nov 13, 2020

bors commented Nov 13, 2020

rust-timer commented Nov 13, 2020

rust-timer commented Nov 13, 2020

oli-obk commented Nov 13, 2020

simonvandel commented Nov 15, 2020

bjorn3 commented Nov 15, 2020

rust-timer commented Nov 15, 2020

bors commented Nov 15, 2020

bors commented Nov 15, 2020

rust-timer commented Nov 15, 2020

rust-timer commented Nov 15, 2020

crlf0710 commented Dec 18, 2020

simonvandel commented Jan 3, 2021

simonvandel commented Nov 10, 2020 •

edited by camelid

Loading