[MIR] Add Dominators to MIR and Add Graph Algorithms #34169

scottcarr · 2016-06-08T23:53:58Z

~~This PR assumes PR #34149 lands.~~

Add generic graph algorithms to rustc_data_structures.

Add dominators and successors to the ~~cache (that currently only holds predecessors).~~ Mir.

rust-highfive · 2016-06-08T23:54:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jroesch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nagisa · 2016-06-09T00:36:18Z

Why is successors’ cache necessary? It is trivial to get up-to-date successors of a block currently. Seems like a super-waste to cache them and whatnot.

scottcarr · 2016-06-09T03:32:59Z

Why is successors’ cache necessary? It is trivial to get up-to-date successors of a block currently. Seems like a super-waste to cache them and whatnot.

I could imagine that looking back at a slightly out of date CFG could be useful in some situation. But, as invalidate is currently implemented that's not possible, so it doesn't seem necessary to cache successors. I'm fine with removing successors from the cache if that is what we want.

nikomatsakis · 2016-06-09T09:28:03Z

src/librustc_data_structures/graph_algorithms/bit_set/mod.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use std::marker::PhantomData;


probably we can use the bit-set that already exists in librustc_data_structures? I think I just copied this one out of there...

nikomatsakis · 2016-06-09T09:36:00Z

It seems like caching successors isn't necessary, though I guess we do take advantage of it for de-duplication. But still -- we could easily do that on the fly in the graph iterator code.

Given the current setup, you can't really look back on invalidated cache data, right? (Though that could be changed easily enough, if we wanted to let you save a snapshot.)

nikomatsakis · 2016-06-09T09:36:36Z

cc @rust-lang/compiler

arielb1 · 2016-06-09T11:19:23Z

@nikomatsakis

Sure you can - clone the cache, e.g. AddCallGuards.

nikomatsakis · 2016-06-10T00:33:20Z

@arielb1 ok, cool. that's what I was going to suggest. seems like I need to have read your PR more carefully! That's the second point I got slightly off. :)

nikomatsakis · 2016-06-10T08:56:52Z

src/librustc/mir/mir_cfg.rs

+    pub fn new<'a, 'tcx>(mir: &Mir, cache: &Cache) -> Self {
+        MirCfg {
+            predecessors: cache.predecessors(mir).clone(),
+            successors: calculate_successors(mir),


Hmm; I expected to implement the graph trait directly on Mir, versus having this intermediate type. Is the idea primarily to have a frozen snapshot for use when computing results, or were there other reasons to introduce MirCfg?

scottcarr · 2016-06-16T12:47:59Z

After working on the implementation and discussing with @nikomatsakis, I/we think it's better to not move the dominators into the cache for now.

Knowing the validity of the cache is a bit tricky. In particular, we envision a MIR transformation that only modifies BasicBlockData::statements, which would mutate the parent Mir but preserve its CFG (i.e. dominators, predecessors, successors, etc).

To me, I'd like to land a PR that adds the CFG algorithms and then we can change the caching/invalidation strategy as we create more MIR analysis/transformations (rather than try to come up with the ideal design now).

(I'm working on a commit that reflects this now.)

arielb1 · 2016-06-20T16:19:53Z

src/librustc/mir/repr.rs

@@ -144,6 +147,13 @@ impl<'tcx> Mir<'tcx> {
    pub fn predecessors_for(&self, bb: BasicBlock) -> Ref<Vec<BasicBlock>> {
        Ref::map(self.predecessors(), |p| &p[bb])
    }
+
+    #[inline]


An invalidate-on-any-change cache could be nice for preventing duplicate uses between passes.

arielb1 · 2016-06-20T21:16:05Z

src/librustc_data_structures/graph_algorithms/dominators/test.rs:25:17: 25:58 error: attempted access of field `vec` on type `&indexed_vec::IndexVec<usize, core::option::Option<usize>>`, but no field with that name was found

src/librustc_data_structures/graph_algorithms/dominators/test.rs:25     assert_eq!(&dominators.all_immediate_dominators().vec[..],

                                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/librustc_data_structures/graph_algorithms/dominators/test.rs:25:5: 29:28 note: in this expansion of assert_eq! (defined in <std macros>)

src/librustc_data_structures/graph_algorithms/dominators/test.rs:48:17: 48:58 error: attempted access of field `vec` on type `&indexed_vec::IndexVec<usize, core::option::Option<usize>>`, but no field with that name was found

src/librustc_data_structures/graph_algorithms/dominators/test.rs:48     assert_eq!(&dominators.all_immediate_dominators().vec[..],

                                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/librustc_data_structures/graph_algorithms/dominators/test.rs:48:5: 51:46 note: in this expansion of assert_eq! (defined in <std macros>)

If you don't want to wait for the world to recompile, you can run compile tests manually by ./x86_64-unknown-linux-gnu/stage0/bin/rustc ./src/librustc_data_structures/lib.rs --test.

bors · 2016-06-21T08:13:34Z

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

nikomatsakis · 2016-06-27T16:27:50Z

@bors r+

bors · 2016-06-27T16:27:51Z

📌 Commit 66d60c7 has been approved by nikomatsakis

bors · 2016-06-27T18:12:26Z

⌛ Testing commit 66d60c7 with merge a0f572e...

[MIR] Add Dominators to MIR and Add Graph Algorithms ~~This PR assumes PR #34149 lands.~~ Add generic graph algorithms to rustc_data_structures. Add dominators and successors to the ~~cache (that currently only holds predecessors).~~ `Mir`.

bors · 2016-06-27T20:59:04Z

This replaces the previous implementation with the simple variant of Lengauer-Tarjan, which performs better in the general case. Performance on the keccak benchmark is about equivalent between the two, but we don't see regressions (and indeed see improvements) on other benchmarks, even on a partially optimized implementation. The implementation here follows that of the pseudocode in "Linear-Time Algorithms for Dominators and Related Problems" thesis by Loukas Georgiadis. The next few commits will optimize the implementation as suggested in the thesis. Several related works are cited in the comments within the implementation, as well. Implement the simple Lengauer-Tarjan algorithm This replaces the previous implementation (from rust-lang#34169), which has not been optimized since, with the simple variant of Lengauer-Tarjan which performs better in the general case. A previous attempt -- not kept in commit history -- attempted a replacement with a bitset-based implementation, but this led to regressions on perf.rust-lang.org benchmarks and equivalent wins for the keccak benchmark, so was rejected. The implementation here follows that of the pseudocode in "Linear-Time Algorithms for Dominators and Related Problems" thesis by Loukas Georgiadis. The next few commits will optimize the implementation as suggested in the thesis. Several related works are cited in the comments within the implementation, as well. On the keccak benchmark, we were previously spending 15% of our cycles computing the NCA / intersect function; this function is quite expensive, especially on modern CPUs, as it chases pointers on every iteration in a tight loop. With this commit, we spend ~0.05% of our time in dominator computation.

rust-highfive assigned jroesch Jun 8, 2016

nikomatsakis reviewed Jun 9, 2016
View reviewed changes

nikomatsakis assigned nikomatsakis and unassigned jroesch Jun 9, 2016

scottcarr force-pushed the dominator-cache branch 2 times, most recently from bf82bc2 to 7cb75b4 Compare June 9, 2016 23:22

scottcarr changed the title ~~[MIR] Cache Dominators and Successors~~ [MIR] Cache Dominators and Add Graph Algorithms Jun 9, 2016

scottcarr force-pushed the dominator-cache branch 2 times, most recently from d7ecb15 to 8ed531b Compare June 9, 2016 23:47

nikomatsakis reviewed Jun 10, 2016
View reviewed changes

scottcarr force-pushed the dominator-cache branch from 40dddfd to d704a3c Compare June 17, 2016 11:17

arielb1 reviewed Jun 20, 2016
View reviewed changes

scottcarr force-pushed the dominator-cache branch 2 times, most recently from bfbcec6 to 03f7a7b Compare June 22, 2016 17:34

scottcarr changed the title ~~[MIR] Cache Dominators and Add Graph Algorithms~~ [MIR] Add Dominators to MIR and Add Graph Algorithms Jun 22, 2016

scottcarr force-pushed the dominator-cache branch 2 times, most recently from 82c4ae9 to bb21968 Compare June 22, 2016 20:35

add control flow graph and algorithms. add dominator to mir

66d60c7

scottcarr force-pushed the dominator-cache branch from d7ef4bc to 66d60c7 Compare June 23, 2016 21:00

bors merged commit 66d60c7 into rust-lang:master Jun 27, 2016

Mark-Simulacrum mentioned this pull request May 6, 2021

Replace dominators algorithm with simple Lengauer-Tarjan #85013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIR] Add Dominators to MIR and Add Graph Algorithms #34169

[MIR] Add Dominators to MIR and Add Graph Algorithms #34169

scottcarr commented Jun 8, 2016 •

edited

Loading

rust-highfive commented Jun 8, 2016

nagisa commented Jun 9, 2016

scottcarr commented Jun 9, 2016

nikomatsakis Jun 9, 2016

nikomatsakis commented Jun 9, 2016

nikomatsakis commented Jun 9, 2016

arielb1 commented Jun 9, 2016

nikomatsakis commented Jun 10, 2016

nikomatsakis Jun 10, 2016

scottcarr commented Jun 16, 2016

arielb1 Jun 20, 2016

arielb1 commented Jun 20, 2016 •

edited

Loading

bors commented Jun 21, 2016

nikomatsakis commented Jun 27, 2016

bors commented Jun 27, 2016

bors commented Jun 27, 2016

bors commented Jun 27, 2016

[MIR] Add Dominators to MIR and Add Graph Algorithms #34169

[MIR] Add Dominators to MIR and Add Graph Algorithms #34169

Conversation

scottcarr commented Jun 8, 2016 • edited Loading

rust-highfive commented Jun 8, 2016

nagisa commented Jun 9, 2016

scottcarr commented Jun 9, 2016

nikomatsakis Jun 9, 2016

Choose a reason for hiding this comment

nikomatsakis commented Jun 9, 2016

nikomatsakis commented Jun 9, 2016

arielb1 commented Jun 9, 2016

nikomatsakis commented Jun 10, 2016

nikomatsakis Jun 10, 2016

Choose a reason for hiding this comment

scottcarr commented Jun 16, 2016

arielb1 Jun 20, 2016

Choose a reason for hiding this comment

arielb1 commented Jun 20, 2016 • edited Loading

bors commented Jun 21, 2016

nikomatsakis commented Jun 27, 2016

bors commented Jun 27, 2016

bors commented Jun 27, 2016

bors commented Jun 27, 2016

scottcarr commented Jun 8, 2016 •

edited

Loading

arielb1 commented Jun 20, 2016 •

edited

Loading