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

Teach rustc --emit=mir #39891

Merged
merged 2 commits into from Mar 23, 2017

Conversation

Projects
None yet
8 participants
@shepmaster
Copy link
Member

shepmaster commented Feb 16, 2017

I'm opening this PR to discuss:

  1. Is this a good idea?
  2. Is this a good implementation?

I'm sure people will have opinions on both points!

This spawned from #31847 (comment), so I figured a prototype implementation could help provide a seed to talk about.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 16, 2017

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@shepmaster shepmaster force-pushed the shepmaster:emit-mir branch from ac66a98 to 7e7e5da Feb 16, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 17, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 17, 2017

So this seems to fall under a broad category of options that we currently confine to -Z but which I think we may want to expose on stable. I'd classify these as "debugging" or "educational" options -- these would include things like the pretty-printer, dumping MIR, profiling information, or other ways to view internal compiler state. The common thread here is that the output would be completely unstable and subject to change -- that is, it is meant for human viewers only (e.g., like diagnostics output).

I sort of suspect that such a family of options should have their own header (rather like -C), and not be conflated with things like --emit, which are used to produce (primarily, anyway) output that can be fed into other tools.

@shepmaster

This comment has been minimized.

Copy link
Member Author

shepmaster commented Feb 17, 2017

I'd classify these as "debugging" or "educational" options

This seems like a reasonable categorization.

the output would be completely unstable and subject to change

Absolutely.

I sort of suspect that such a family of options should have their own header (rather like -C)

This makes sense to me

not be conflated with things like --emit, which are used to produce (primarily, anyway) output that can be fed into other tools.

I suspect I fall into the target group of that "primarily" 😇 . My personal stake is that I'd like to have a stable and unified way of accessing the final MIR, ASM, and LLVM-IR for the playground. In that context, all three of these are debugging/educational, often for the purposes of "how does that look when optimized?".

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 17, 2017

We could probably find a better name for --pretty (or rather, --unpretty).
In fact, we have --print which can output a dozen useful strings.
What if we gave that -P and also used it for pretty-printing? That is, -P pretty-source or something.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 21, 2017

@eddyb seems reasonable to me. I'm also not opposed to using --emit for this purpose, I guess, but it does seem like there is a definite distinction between "object code" and "MIR" or "pretty-printed HIR".

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 27, 2017

So I was looking into --print. The current list of things to print seems (to me) to be targeting tool consumption, right?

    --print [crate-name|file-names|sysroot|cfg|target-list|target-cpus|target-features|relocation-models|code-models|target-spec-json]
                        Comma separated list of compiler information to print

Is that then the right option? Does it matter if we mix "tool"-targeting things from "human"-targeting things under one option? It feels like it will encourage misuse, but perhaps not.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 27, 2017

FWIW --print is generally "this goes to stdout" where --emit is "this goes to a file", and this seems like it's intending to be the latter?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 28, 2017

@alexcrichton I definitely agree --print is better than --emit. Just wondering if it's the right thing to use, in terms of discouraging people from casually thinking the output is stable. Maybe it's good enough.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 28, 2017

Or maybe those things aren't especially stable either?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 28, 2017

I'd personally be ok with a giant header comment saying "THIS OUTPUT IS NOT STABLE IT'S JUST MEANT FOR DEBUGGING, IT WILL CHANGE WITHOUT NOTIFICATION" or something like that.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 2, 2017

@alexcrichton where would such a comment go? Would it apply to all the things that --print uses?

Right now the full list shows up in --help. I was thinking maybe it should not, and we should instead have some way to get the list. This list could then make clear (perhaps with an asterix) those items where the output format is not, and will likely never be, stable. But at the end of the day I kind of just feel is has to be written somewhere that is not TOO obscure, and it's fine. Which I think is your point.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 2, 2017

Oh sorry I misread what you preferred. I assumed that you preferred --emit (like me) in which case such a comment would go in the file.

If you prefer --print then yeah there's not really a place for such a comment.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 2, 2017

@alexcrichton oh, like, put it in the output itself? That's...kind of a good idea. =)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 2, 2017

Just add something like to the front:

// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.

Regarding --emit vs --print I don't honestly care that much.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 2, 2017

@nikomatsakis precisely yeah (that's what I was thinking)

@shepmaster shepmaster force-pushed the shepmaster:emit-mir branch 2 times, most recently from fb8f280 to 476a1b9 Mar 3, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 3, 2017

@alexcrichton I actually really like that =)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 3, 2017

@rfcbot fcp merge

I propose we adopt @alexcrichton's suggestion here, and support --emit mir (and other such unstable-y things) but we prefix the output with some comments warning people from relying on any particular detail of the format. We can do the same with the pretty printer, etc. (Feel free to object if you prefer something other than --emit or have any other concerns. I'm just trying to move things along here.)

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 3, 2017

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@shepmaster shepmaster force-pushed the shepmaster:emit-mir branch from 476a1b9 to 1e8afde Mar 4, 2017

@shepmaster

This comment has been minimized.

Copy link
Member Author

shepmaster commented Mar 4, 2017

Because I found it equally amusing and informative, I added @nikomatsakis suggested phrasing verbatim.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 20, 2017

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

@shepmaster shepmaster force-pushed the shepmaster:emit-mir branch from 1e8afde to 00ba48c Mar 21, 2017

@shepmaster

This comment has been minimized.

Copy link
Member Author

shepmaster commented Mar 21, 2017

Nothing scarier than your little ol' PR getting a merge conflict from a hulking +2,797 −3,169 PR. Thankfully, it didn't seem to have any actual conflicts...

@bjorn3

This comment has been minimized.

Copy link
Contributor

bjorn3 commented Mar 21, 2017

To prevent tools from using this you could put spaces/dots/underscores at random places.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Mar 21, 2017

@bjorn3 Any half-decent lexer can remove extra spaces, and if it impedes a lexer, it also does humans.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 21, 2017

@shepmaster shepmaster force-pushed the shepmaster:emit-mir branch from 00ba48c to e341a94 Mar 21, 2017

@shepmaster shepmaster force-pushed the shepmaster:emit-mir branch from e341a94 to 4ddedf7 Mar 22, 2017

@shepmaster

This comment has been minimized.

Copy link
Member Author

shepmaster commented Mar 22, 2017

@nikomatsakis thank you for the heads up. The build is green again.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 22, 2017

Mm I'm going to r+ even though the FCP has not yet expired. I think we've had enough time here.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 22, 2017

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 22, 2017

📌 Commit 4ddedf7 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 23, 2017

Rollup merge of rust-lang#39891 - shepmaster:emit-mir, r=nikomatsakis
Teach rustc --emit=mir

I'm opening this PR to discuss:

1. Is this a good idea?
1. Is this a good implementation?

I'm sure people will have opinions on both points!

This spawned from rust-lang#31847 (comment), so I figured a prototype implementation could help provide a seed to talk about.

bors added a commit that referenced this pull request Mar 23, 2017

Auto merge of #40752 - frewsxcv:rollup, r=frewsxcv
Rollup of 6 pull requests

- Successful merges: #39891, #40518, #40542, #40617, #40678, #40696
- Failed merges:

@bors bors merged commit 4ddedf7 into rust-lang:master Mar 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shepmaster shepmaster deleted the shepmaster:emit-mir branch Mar 23, 2017

nateozem pushed a commit to nateozem/rust that referenced this pull request Apr 14, 2017

add 'mir' as part of the --emit flag list in rustc --help menu and ma…
…n doc.

This is added because 'rustc' can now generate MIR (referencing to
"Teach rustc --emit=mir rust-lang#39891").

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 15, 2017

Rollup merge of rust-lang#41306 - nateozem:support/docs-mir, r=frewsxcv
add 'mir' to rustc help menu and man doc

add 'mir' to '--emit' flag list for 'rustc'.
This is added because 'rustc' can now generate MIR (referencing to
"Teach rustc --emit=mir rust-lang#39891").
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.