[MIR] Generalize the depth first search logic and use it in all of SimplifyCfg. #30239

gereeter · 2015-12-06T16:39:01Z

[MIR] Generalize the depth first search logic and use it in all of SimplifyCfg.

…mplifyCfg

rust-highfive · 2015-12-06T16:39:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

dotdash · 2015-12-06T18:10:31Z

src/librustc_mir/transform/util.rs

@@ -21,6 +21,31 @@ pub fn update_basic_block_ids(mir: &mut Mir, replacements: &[BasicBlock]) {
    }
 }

+/// Run a function on every reachable basic block, then delete any unreachable blocks.
+/// The function given should not add or remove and blocks from the control flow graph.


gereeter · 2015-12-06T19:21:31Z

Typo fixed.

nikomatsakis · 2015-12-14T23:37:13Z

src/librustc_mir/transform/simplify_cfg.rs

@@ -126,7 +105,6 @@ impl<'tcx> MirPass<'tcx> for SimplifyCfg {
        while changed {
            changed = self.simplify_branches(mir);
            changed |= self.remove_goto_chains(mir);
-            self.remove_dead_blocks(mir);


Huh. I'm not sure if it is better to compress the blocks twice for each round. Seems like it might be slower overall?

Also, it seems like this might take longer to reach a fixed point, since in some cases (e.g. simplify_branches) you may now have to do one step to modify the terminator and another round to remove the block, and then a final round, whereas before I think you would have finished in two?

TL;DR it may be time to start thinking about some compile-time measurements here :)

nikomatsakis · 2015-12-18T10:16:12Z

@gereeter thoughts on this comment of mine:

...it seems like this might take longer to reach a fixed point, since in some cases (e.g. simplify_branches) you may now have to do one step to modify the terminator and another round to remove the block, and then a final round, whereas before I think you would have finished in two?

gereeter · 2015-12-19T03:14:26Z

I don't completely understand what you are worried about - the only difference execution-wise with this change is the dead blocks are now removed twice per round, not a single time. This should only improve convergence by making any optimization opportunities more obvious earlier.

nikomatsakis · 2015-12-20T11:41:36Z

@gereeter

I don't completely understand what you are worried about - the only difference execution-wise with this change is the dead blocks are now removed twice per round, not a single time. This should only improve convergence by making any optimization opportunities more obvious earlier.

Man, coming back to this, I am not sure what I was worried about either when it comes to convergence.

However, I AM concerned that adjusting basic blocks has a kind of fixed cost and it would be cheaper to do it once per round once we know the full set of things to be removed. I guess it's hard to know without measuring. Anybody on @rust-lang/compiler have an opinion? @dotdash?

Aatch · 2015-12-22T01:59:42Z

I can't see how this would improve convergence time. Unreachable blocks are unreachable, whether or not they're removed from the MIR, so they aren't going to be visited during the depth-first search either way. The only reason I can see for removing the blocks each iteration before was because it was iterating over all blocks, reachable or otherwise, so removing them meant they weren't looked at unnecessarily.

Since this code also changes the sub-passes to do a preorder traversal, there is no advantage to removing unreachable blocks before we hit a fixpoint any more. Therefore, I think we should keep the remove_dead_blocks function and run it just before returning.

gereeter · 2015-12-22T02:35:30Z

@Aatch At this point, you are right that removing the blocks in the middle has very little effect. Once #30238 merges, however, the predecessor map can be messed up by dead blocks, slowing convergence if they aren't pruned before producing the map. I'm not sure if that's a good enough reason to do it here, though.

bors · 2016-01-06T15:01:08Z

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

alexcrichton · 2016-01-27T00:30:57Z

(ping)

Does this just need a rebase? Comments to address?

(trying to help clear out the queue)

nikomatsakis · 2016-01-28T17:21:11Z

Yeah, I'm not really sure. I remain mildly uncomfortable with this change, but I've kind of forgotten exactly why. I guess I still am just not sure that this is an improvement. Given that we did not land #30238, as @Aatch suggested an alternative approach, I think we ought to close this one too (since the two seem to be linked). Closing therefore. @gereeter let me know if I'm mis-remembering things, and thanks for the PR in any case.

gereeter · 2016-01-30T02:17:28Z

Sorry for not replying earlier. This definitely should be changed to take into account @Aatch's suggestions, and this should probably be combined with #30238. Unfortunately, I've been busy and haven't gotten around to improving this.

[MIR] Generalize the depth first search logic and use it in all of Si…

78ada84

…mplifyCfg

rust-highfive assigned nikomatsakis Dec 6, 2015

[MIR] Add documentation to adjust_reachable_basic_blocks

b59f377

dotdash reviewed Dec 6, 2015
View reviewed changes

gereeter mentioned this pull request Dec 6, 2015

[MIR] Enhance the SimplifyCfg pass to merge consecutive blocks #30238

Closed

[MIR] Fix typo in adjust_reachable_basic_blocks' docs

8529978

nikomatsakis reviewed Dec 14, 2015
View reviewed changes

pnkfelix changed the title ~~[MIR] Generalize the depth first search logic and use it in all of Si…~~ [MIR] Generalize the depth first search logic and use it in all of SimplifyCfg. Dec 15, 2015

nikomatsakis closed this Jan 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIR] Generalize the depth first search logic and use it in all of SimplifyCfg. #30239

[MIR] Generalize the depth first search logic and use it in all of SimplifyCfg. #30239

gereeter commented Dec 6, 2015

rust-highfive commented Dec 6, 2015

dotdash Dec 6, 2015

gereeter commented Dec 6, 2015

nikomatsakis Dec 14, 2015

nikomatsakis Dec 14, 2015

nikomatsakis Dec 14, 2015

nikomatsakis commented Dec 18, 2015

gereeter commented Dec 19, 2015

nikomatsakis commented Dec 20, 2015

Aatch commented Dec 22, 2015

gereeter commented Dec 22, 2015

bors commented Jan 6, 2016

alexcrichton commented Jan 27, 2016

nikomatsakis commented Jan 28, 2016

gereeter commented Jan 30, 2016

[MIR] Generalize the depth first search logic and use it in all of SimplifyCfg. #30239

[MIR] Generalize the depth first search logic and use it in all of SimplifyCfg. #30239

Conversation

gereeter commented Dec 6, 2015

rust-highfive commented Dec 6, 2015

dotdash Dec 6, 2015

Choose a reason for hiding this comment

gereeter commented Dec 6, 2015

nikomatsakis Dec 14, 2015

Choose a reason for hiding this comment

nikomatsakis Dec 14, 2015

Choose a reason for hiding this comment

nikomatsakis Dec 14, 2015

Choose a reason for hiding this comment

nikomatsakis commented Dec 18, 2015

gereeter commented Dec 19, 2015

nikomatsakis commented Dec 20, 2015

Aatch commented Dec 22, 2015

gereeter commented Dec 22, 2015

bors commented Jan 6, 2016

alexcrichton commented Jan 27, 2016

nikomatsakis commented Jan 28, 2016

gereeter commented Jan 30, 2016