Skip to content
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

Use DataflowResultsCursor instead of FlowAtLocation #62315

Closed
tmandry opened this issue Jul 2, 2019 · 3 comments
Closed

Use DataflowResultsCursor instead of FlowAtLocation #62315

tmandry opened this issue Jul 2, 2019 · 3 comments
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Jul 2, 2019

#61922 introduced DataflowResultsCursor, which provides a stateful caching interface atop FlowAtLocation.

It would be better to use DataflowResultsCursor from more places, both because it's a more convenient / less error-prone interface, and because it's best to have more than one code path exercising the same code. It's also awkward trying to support both interfaces at once (the cursor implementation would be cleaner if we didn't have to).

The main drawback to using it is that there will be extra state/branching in places where we don't need it.

cc @eddyb who asked me to open this, and (along with @nikomatsakis) helped provide inspiration for DataflowResultsCursor
cc @ecstatic-morse @pnkfelix

@jonas-schievink jonas-schievink added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Jul 2, 2019
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jul 2, 2019

There's a few places in librustc_mir which implement the same functionality:

state_for_location can also be replaced easily. However, DataflowResultsConsumer is a bit more complex because it allows results to be zipped together (this is the purpose of FlowsAtLocation, although it claims to be for "cartesian products"). We could either rewrite all the consumers of this API to use the cursor API, or make DataflowResultsCursor the underlying implementation of DataflowResultsConsumer like so:

trait DataflowResultsVisitorEngine<'a> {
    type State: 'a;
    fn reset_to_start_of_block(&'a mut self, block: BasicBlock);
    fn seek(&'a mut self, loc: Location);
    fn get(&'a self) -> Self::State;
}

// Tuples of `DataflowResultsCursor`s can implement `DataflowResultsVisitorEngine`
impl<'a, A, B> DataflowResultsVisitorEngine<'a> for (DataflowResultsCursor<A>, DataflowResultsCursor<B>) {
    type State = (&'a BitSet<A::Idx>, &'a BitSet<B::Idx>);
    /* ... */
}

// New version of `DataflowResultsConsumer`
trait DataflowResultsVisitor<'a, 'tcx> {
      type Engine: DataflowResultsVisitorEngine<'a>;

    // Observation Hooks: override (at least one of) these to get analysis feedback.
    fn visit_block_entry(&mut self,
                         _bb: BasicBlock,
                         _flow_state: Self::Engine::State) {}

    fn visit_statement_entry(&mut self,
                             _loc: Location,
                             _stmt: &Statement<'tcx>,
                             _flow_state: Self::Engine::State) {}

    fn visit_terminator_entry(&mut self,
                              _loc: Location,
                              _term: &Terminator<'tcx>,
                              _flow_state: Self::Engine::State) {}

    fn visit(&mut self, cursor: &'a mut Self::Engine) {
         todo!()
    }
}

DataflowResultsVisitorEngine is basically a streaming iterator with a few extra methods. You could almost do this with a generic StreamingIterator trait with a zip method, but that means StreamingIterator::next would have to visit statements, terminators, and the start of a block.

@jonas-schievink
Copy link
Contributor

Seems like this is done, right?

@ecstatic-morse
Copy link
Contributor

Resolved by #69644.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants