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

[NLL] extend mir-dump to dump out the values of each region #44872

Closed
nikomatsakis opened this issue Sep 26, 2017 · 4 comments
Closed

[NLL] extend mir-dump to dump out the values of each region #44872

nikomatsakis opened this issue Sep 26, 2017 · 4 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Building on #44870, once we have a vector of values, we will want to know what they are. I think the best idea is to include that information in the MIR dump. The MIR pretty-printing code is found in pretty.rs.

Currently, this is invoked by the "Dump MIR" code found in dump_mir.rs. This is part of the MIR pass setup -- basically this visitor gets invoked in between every MIR pass. However, we don't really want to touch this "Dump MIR" code -- this is because it runs in between passes, and the intention with the NLL inference is that the non-lexical lifetime information will be allocated and destroyed by the NLL pass itself, and hence it will be freed by the time this code runs.

Instead, what I think we ought to do is invoke the pretty-printer directly from within the NLL pass itself. This is easy enough, we just have to invoke the dump_mir() function independently from the pass infrastructure, much like the mir-builder function does. Basically inserting a line like the following right in between line 142 and line 143 in NLL:

 mir_util::dump_mir(tcx, None, "nll", &0, source, &mir);

However, this still won't do anything particularly interesting right now, because it doesn't have access to the region values! So I think we should extend dump_mir to take an extra argument, or perhaps a callback, that lets us add extra information into the dump. Let's suppose we take the callback approach. Then we might do something like:

mir_util::dump_mir(tcx, None, "nll", &0, source, &mir, |out| {
    for (index, value) in self.region_values.iter().enumerate() {
        write!(out, "Region {:03}: {:?}", index, value)?;
    }
    Ok(())
});

and we would modify dump_mir() to say something like:

pub fn dump_mir<'a, 'tcx, F>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                          pass_num: Option<(MirSuite, MirPassIndex)>,
                          pass_name: &str,
                          disambiguator: &Display,
                          source: MirSource,
                          mir: &Mir<'tcx>,
                          extra_data: F)
where
    F: FnMut(&mut Write) -> io::Result<()>
{
    ...
}

this callback extra_data would be threaded down to the code that actually does the writing and invoked at the end or something.

One minor annoyance I see is that dump_mir also iterates over the "promoted" values associated with Mir. These are basically constant expressions that got extracted. We probably don't want to be dumping those. We could add a flag to disable this loop. But I think better yet would be to remove the loop altogether, and move it to this caller here, since I think that is the only one that actually wants this loop. That is, we would tranformat that code from:

        if mir_util::dump_enabled(tcx, pass_name, source) {
            mir_util::dump_mir(tcx,
                               Some((suite, pass_num)),
                               pass_name,
                               &Disambiguator { is_after },
                               source,
                               mir);
    }

to something like:

if mir_util::dump_enabled(tcx, pass_name, source) {
    mir_util::dump_mir(tcx,
        Some((suite, pass_num)),
        pass_name,
        &Disambiguator { is_after },
        source,
        mir);
    for (index, promoted_mir) in mir.promoted.iter_enumerated() {
        let promoted_source = MirSource::Promoted(source.item_id(), index);
        mir_util::dump_mir(tcx,
            Some((suite, pass_num)),
            pass_name,
            &Disambiguator { is_after },
            promoted_source,
            promoted_mir);
   }
}
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-nll E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Sep 26, 2017
@nikomatsakis
Copy link
Contributor Author

@chrisvittal has been working on this. During discussion in WG-compiler-nll, we came up with some nice ideas. For example, the callback ought to be invoked many times, with an argument indicating where in the control-flow graph we are located. I am imagining:

enum Where {
    /// We have not yet started dumping the control-flow graph, but we are about to.
    /// This is e.g. where the types of local variables appear and so forth.
    BeforeCFG,

    /// We just finished dumping the control-flow graph. This is right before the end of the file.
    AfterCFG,

    /// We are about to start dumping the given basic block.
    BeforeBlock(BasicBlockIndex),

    /// We are just about to dump the given statement or terminator.
    InCFG(Location),
}

This way we can also dump information about liveness, showing for example what is live on entry to each block, what is considered borrowed, and other information like that.

@nikomatsakis
Copy link
Contributor Author

I imagine that before/after CFG would be invoked before and after this line, respectively.

Before block would be invoked right before this line, which starts the basic block, and the statement callback would be called before this line, which prints a statement and before this line, which prints the terminator.

@aidanhs aidanhs added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Sep 28, 2017
bors added a commit that referenced this issue Oct 13, 2017
…akis

Extend mir dump to dump each region

Building on #44878, implement the feature discussed in #44872.

Through discussions on the WG-nll-gitter, @nikomatsakis and I decided to implement this by extending `dump_mir` and all functions that it calls to take a callback of signature `FnMut(PassWhere, &mut Write) -> io::Result<()>` where `PassWhere` is an enum that represents possible locations that we may want to print out extra data in the process of dumping the MIR.

I'm not particularly wedded to the name `PassWhere`, but I felt that simply calling the enum `Where` wasn't the right thing to name it.

This work depends strongly on #44878, and should be rebased on the final version of that tree, whatever that may be.
@Lorgum
Copy link

Lorgum commented Oct 24, 2017

asdasd

@nikomatsakis
Copy link
Contributor Author

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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

4 participants