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

Add RUSTC_EMIT option to pass on --emit to crates during bootstrap #108365

Closed
wants to merge 2 commits into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Feb 22, 2023

This makes it a bit more convenient to inspect the generated code of rustc crates. I also have an upstream patch to add demangling support to llvm-reduce which pairs well with this to make the LLVM IR output more readable.

@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2023

r? @albertlarsan68

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 22, 2023
@albertlarsan68
Copy link
Member

I have a few remarks:

  • Have you tested that rustc is built correctly while dumping those representations?
  • Is there another way to do this currently/maybe it can be useful to generalize this principle to the other emit values (MIR, HIR, THIR, etc.)
  • Can you add documentation in the right places (cc @ozkanonur for guidance)

Note for future/follow-up/now:

  • Allow to set exactly which format is to be output, I fear that with all the disk gets filled rapidly (maybe unfounded fear but having 6 copies of the same program is not the best thing to have (currently the code itself, incremental compilation data, and final objects, with this PR add ASM output, LLVM IR and bitcode)).

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 27, 2023

x.py build library seems to work at least. I haven't done any more extensive testing.

The --emit option supports MIR, so that would be easy to add.

I could add the ability to specify the output formats with a syntax like rustc_arena=mir,llvm-ir;rustc_infer=llvm-bc.

@albertlarsan68
Copy link
Member

x.py build library seems to work at least. I haven't done any more extensive testing.

Can you test building a stage 2 compiler and using it?
If it is too expensive for you, I can do it.

The --emit option supports MIR, so that would be easy to add.

Yes please!

I could add the ability to specify the output formats with a syntax like rustc_arena=mir,llvm-ir;rustc_infer=llvm-bc.

This would be great!
If you add MIR support, this addition would be very useful.

@onur-ozkan
Copy link
Member

Can you add documentation in the right places (cc @ozkanonur for guidance)

The related documentation place for this context points to src/bootstrap/README.md file. Later on we can provide helpful informations in dev-guide, but that's outside of this PR's scope.

@Zoxc Zoxc changed the title bootstrap: Add RUSTC_DUMP option to dump LLVM IR, LLVM bitcode, and assembly for crates Add RUSTC_EMIT option to pass on --emit to crates during bootstrap Mar 2, 2023
@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 2, 2023

I've renamed the environmental variable to RUSTC_EMIT. It now uses uses the syntax I proposed and it just passes the format on to rustc so mir is also supported.

The dev guide seems to be the right place to add documentation as src\bootstrap\README.md explicitly says it's not usage documentation.

Copy link
Member

@albertlarsan68 albertlarsan68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post a PR to the rustc dev guide, and apply my comments if you want.

src/bootstrap/bin/rustc.rs Show resolved Hide resolved
"llvm-bc" => "bc",
"asm" => {
if target.map_or(false, |target| target.starts_with("x86")) {
cmd.arg("-Cllvm-args=-x86-asm-syntax=intel");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up, make this configurable to be able to output ATT syntax.

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 6, 2023

I've opened a PR to add documentation.

@albertlarsan68 albertlarsan68 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2023
@albertlarsan68
Copy link
Member

It would also be great if you add an entry to the changelog for bootstrap (located in src/bootstrap/CHANGELOG.md IIRC).

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 15, 2023

Done.

@bors
Copy link
Contributor

bors commented Mar 15, 2023

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

@jyn514
Copy link
Member

jyn514 commented Mar 17, 2023

Why not use RUSTFLAGS in combination with x clean rustc_data_structures (or whatever crate you're interested in)?

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 17, 2023

RUSTFLAGS is not per crate and I don't want to dig through cargo/rustc's output files to find the emitted files.

@albertlarsan68 albertlarsan68 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 18, 2023
@albertlarsan68
Copy link
Member

It seems like rustc does not respect the output dir everytime. For example, LLVM IR and ASM do still output in the "normal" target directory, for example to build/host/stage1-std/x86_64-unknown-linux-gnu/release/deps/compiler_builtins-acc40b1213caf41b.compiler_builtins.6d7cb61c-cgu.0.rcgu.s for the ASM output for compiler_builtins.

@albertlarsan68 albertlarsan68 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2023
@albertlarsan68 albertlarsan68 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2023
@bors
Copy link
Contributor

bors commented May 9, 2023

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

@bjorn3
Copy link
Member

bjorn3 commented May 11, 2023

That may be a result of the hacks we use to force compiler-builtins to build with multiple codegen units. --emit asm/llvm-ir/llvm-bc/obj normally forces a single codegen unit for back compat reasons with when we didn't use multiple codegen units for a single crate yet.

@jyn514
Copy link
Member

jyn514 commented May 18, 2023

In the meantime, you could use x clean rustc_foo && MAGIC_EXTRA_RUSTFLAGS=--emit=... to avoid invalidating the build cache and only pass the flags to a single crate.

@JohnCSimon
Copy link
Member

triage:
@Zoxc can you please fix the merge conflict

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jun 25, 2023

I think I'll close this for now. It doesn't interact well with multiple CGUs and LTO. It might be better to base it on save-temps or overhaul the IR emitting command line option.

@Zoxc Zoxc closed this Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants