Skip to content

/rust

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.

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

[WIP] [MIR] Generic lattice-based dataflow framework, rebased #34164

Closed
wants to merge 24 commits into from

Conversation

Reviewers
No reviews
Assignees

@Aatch Aatch

Labels
None yet
Projects
None yet
Milestone
No milestone
9 participants
@gereeter @rust-highfive @nagisa @bors @pcwalton @nikomatsakis @luqmana @arielb1 @Aatch
@gereeter
Copy link
Contributor

gereeter commented Jun 8, 2016

This is a rebased and extended version of @nagisa's #33628. It could still do with at least cleanup and optimization, and there are extensions and changes that @nagisa has planned and that I have planned. However, I believe that I have fixed any bugs it causes, and this version successfully bootstraps locally.

nagisa and others added some commits May 14, 2016

@nagisa @gereeter
Distinct CFG type for MIR, traversals to librustc
bb183e2
@nagisa @gereeter
Impl a generic lattice-based DF framework 
Only a forward one so far
d7a0466
@nagisa @gereeter
Implement a ACS propagation pass 
ACS here stands for Alias-Constant-Simplify. This propagation pass is a composition of three
distinct dataflow passes: alias-propagation, constant-propagation and terminator simplification.

This pass also serves to showcase the features of the framework. Most notably the terseness,
composability and the capability to interleave analysis and transformations.
060d840
@gereeter
Remove use of specialization in Lattice to avoid use of unstable feat… 
…ures
e20d181
@gereeter
Remove DataflowPass
ea689a0
@gereeter
Add &self arguments to Transfer and Rewrite and start taking them by … 
…value instead of by type
c1bb060
@gereeter
Remove unused and buggy support for dataflow passes that introduce mu… 
…ltiple statements
52eff47
@gereeter
Let ar_forward generate its own queue
6413fe6
@gereeter
Change the fully capitalized ACS to the partially capitalized Acs, ma… 
…tching the rest of rustc
0756739
@gereeter
Fix various nits in MIR Dataflow
a0eebd3
@gereeter
Actually rewrite constants in AcsPropagate
7cbcb4b
@gereeter
Remove some unnecessary `pub`s in AcsPropagate
4af1473
@gereeter
Invalidate values in AcsLattice that are overwritten, either by a wri… 
…te to a projected lvalue, or through the destination of a call. Projected lvalues are now entirely not tracked.

With --enable-orbit, everything bootstraps except for denied warnings.
ad37533
@gereeter
Temporarily completely disable Backward dataflow to fix unused varian… 
…t warning
9b3ecda
@gereeter
Rewrite AcsLattice to be more correct
b0fbbae
@gereeter
Further correctness improvements to AcsRewrite
7f1d4c8
@gereeter
Remove unused import left over from rebase
cfcf7cd
@gereeter
Properly reset basic blocks optimized based on overspeculative inform… 
…ation
ae0c91c
@gereeter
Implement MIR's CFG::swap correctly
ffc2406
@gereeter
Mutably update blocks when analyzing instead of creating new ones. ru… 
…stc bootstraps!
Loading status checks…
3bd906c

@rust-highfive rust-highfive assigned Aatch Jun 8, 2016

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Jun 8, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (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.
@gereeter
Fix make tidy
Loading status checks…
07d073e
@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jun 8, 2016

NB: CFG part of bb183e2 will not be necessary anymore once #34149 lands.
@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jun 8, 2016

Also, mutable updates of original block are somewhat incompatible with my plans to make it possible to replace any statement with an arbitrary subgraph, which is why I used to create a new block and then swap in the first place.
@gereeter

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gereeter commented Jun 8, 2016

I don't think mutable updates are necessarily incompatible with replacement by an arbitrary subgraph - the design I was envisioning was for the StatementChange variant to simply contain two basic blocks, an entry block and an exit block. The rewrite pass is responsible for creating any new blocks. In ar_forward, when this is seen, the tail of the current block is copied to the exit block, the current fact is recorded for the entry block, and a new Goto terminator is stuck on. This shouldn't confuse the fixpoint loop, as basic block numbers and meanings remain stable - it just has to be notified that there are new basic blocks.
@gereeter

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gereeter commented Jun 8, 2016

Note that although Travis passed, the debuginfo tests, which Travis skips, failed locally.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 8, 2016

☔️ The latest upstream changes (presumably #33989) made this pull request unmergeable. Please resolve the merge conflicts.
src/librustc_mir/transform/acs_propagate.rs

struct AcsPropagateTransfer;

fn base_lvalue<'a, 'tcx>(mut lval: &'a Lvalue<'tcx>) -> &'a Lvalue<'tcx> {

This comment has been minimized.

Sign in to view
Copy link
@luqmana

luqmana Jun 10, 2016

Member

It'd be reasonable to just add this as a method on Lvalue since other transformations will almost certainly find it useful as well.
@pcwalton

This comment has been minimized.

Sign in to view
Copy link
Contributor

pcwalton commented Jun 12, 2016

Does this effectively implement part of #32966?
@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jun 12, 2016

@pcwalton yes.
@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jun 12, 2016 •
edited

@gereeter I’m essentially done with my schoolwork now so I’ll be fetching your PR and begin working onto it. Thanks for your contribution!
src/librustc_mir/transform/remove_dead_blocks.rs
@@ -72,11 +72,11 @@ fn retain_basic_blocks(mir: &mut Mir, keep: &BitVector) {
if alive_index != used_blocks {
// Swap the next alive block data with the current available slot. Since alive_index is
// non-decreasing this is a valid operation.
mir.basic_blocks.swap(alive_index, used_blocks);
mir.cfg.basic_blocks.swap(alive_index, used_blocks);

This comment has been minimized.

Sign in to view
Copy link
@arielb1

arielb1 Jun 16, 2016

Contributor

I prefer having RemoveDeadBlocks being a "GC" pass and doing the swapping "manually", with other passes not removing BBs.

@scottcarr scottcarr referenced this pull request Jun 17, 2016

Open

Do move forwarding on MIR #32966

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jun 22, 2016

@nagisa @gereeter what is status of this PR, and which optimizations does it implement? I haven't had a chance to look at it yet (I admit, I was kind of waiting until @nagisa was around again, just because of lack of time...) but I'd like to try and do so now (though I'm also sort of on vacation -- trying to at least keep up with PRs though). But I see some comments suggesting that @nagisa may be opening another version?
@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jun 22, 2016 •
edited

My own most up-to-date branch is here. It is based on this PR, so if you’re looking at anything, my suggestion is to look at that.

what is status of this PR, and which optimizations does it implement?

This PR cannot be merged as is. It does not differ very much from what I envision the thing will become in the end, though, so it certainly should be fine to do a shallow review to get familiar with the approach if there’s a desire to do so.

My goal is to have forward move propagation and constant propagation passes implemented as a showcase of the framework (@greeter spent a lot of time making sure they are correct). This PR has those two passes implemented, but they still don’t quite work (or shouldn’t work) due to missing cleanup (liveness pass) and speculative analysis support (what @arielb1 noticed was missing during their review of the PR).

Ultimately, given this cannot be merged, is out of date and is way past the request-for-comments date (since I’m actively working on it again), I suggest to close this PR.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jul 1, 2016

Ultimately, given this cannot be merged, is out of date and is way past the request-for-comments date (since I’m actively working on it again), I suggest to close this PR.

OK.

@nikomatsakis nikomatsakis closed this Jul 1, 2016

@nagisa nagisa referenced this pull request Aug 11, 2016

Closed

[MIR] Dataflow framework, constant propagation and dead code elimination #35608

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.