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

Switch to MIR-based translation by default. #34096

Merged
merged 7 commits into from Aug 2, 2016
Merged

Switch to MIR-based translation by default. #34096

merged 7 commits into from Aug 2, 2016

Conversation

@eddyb
Copy link
Member

@eddyb eddyb commented Jun 5, 2016

This patch makes -Z orbit default to "on", which means that by default, functions will be translated from Rust to LLVM IR through the upcoming MIR backend, instead of the antiquated AST backend.

This switch is made possible by the recently merged #33622, #33905 and smaller fixes.

If you experience any issues, please file a report for each of them. You can switch to the old backend to work around problems by either setting RUSTFLAGS="-Zorbit=off" or by annotating specific functions with #[rustc_no_mir] (which requires #![feature(rustc_attrs)] at the crate-level).

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

cc @rust-lang/compiler

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Jun 5, 2016

r? @pnkfelix

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

Loading

@nagisa
Copy link
Member

@nagisa nagisa commented Jun 5, 2016

@bors r+

I feel like approving this PR for nightly makes a lot of sense. We’ve been testing and improving MIR for a while now and making it default on nightly will get MIR to many more hands than we had before so far.

One concern I have that as the PR is now its pretty easy to forget to flip the true to false before the branch to beta if we figure out that we are not ready to enable this yet. On the other hand, reversing enablement is as easy of an one-line-change, so @eddyb convinced me that it will be fine.

I hope time this PR will need to go through the queue will be enough for all relevant parties to see this.

Loading

@bors
Copy link
Contributor

@bors bors commented Jun 5, 2016

📌 Commit ce179ea has been approved by nagisa

Loading

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 5, 2016

Sure. We have to flip that switch someday, and it's not like we will discover any new bugs with the switch set to off.

On the other hand, I would prefer a crater run with LLVM assertions enabled. I would prefer that to a dump of confusing ICE reports.

There are only 3 PRs left on the queue, and our build times improved since the opt-mir builds are no longer holding things up - this PR may go today UTC, and I am not sure @nikomatsakis is online.

cc @rust-lang/compiler

Loading

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 5, 2016

hold hold hold
@bors r-

canceling pending a crater run

Loading

@nagisa
Copy link
Member

@nagisa nagisa commented Jun 5, 2016

… and niko’s seal of approval, of course, which implies

r? @nikomatsakis

Loading

@alexbool
Copy link
Contributor

@alexbool alexbool commented Jun 5, 2016

Whoa, how about #33828?

Loading

@nagisa
Copy link
Member

@nagisa nagisa commented Jun 5, 2016

I personally do not see performance regressions as a blocker to enable MIR in nightly by default, because otherwise we will never end up enabling it at all. Rather, enabling MIR by default on nightly would serve to help collect more reports on issues with MIR (both performance and correctness) and speed up fixing said issues. Perhaps all within the same cycle.

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 5, 2016

@alexbool Those numbers predate non-zeroing drops. I can take a look, I suppose, maybe it has a completely different reason.

Loading

@alexbool
Copy link
Contributor

@alexbool alexbool commented Jun 5, 2016

@eddyb How is that related to non-zeroing drops? If I get it right, MIR and old code both use the same zeroing drops in that benchmark, so the competion is fair. (Please tell me if I am wrong)

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 5, 2016

@alexbool Old trans can actually handle some static drop/unconditional move cases better than MIR trans did before #33622 - MIR trans used to generate a ton of memsets with the "uninitialized" and "dropped" byte patterns instead.

Loading

@alexbool
Copy link
Contributor

@alexbool alexbool commented Jun 5, 2016

Thanks, sounds convincing

Loading

@eddyb eddyb mentioned this pull request Jun 5, 2016
@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 5, 2016

The Travis failure is strange, although likely legitimate:

run doc-book-lang-items [x86_64-unknown-linux-gnu]

running 1 test
test _0 ... FAILED

failures:

---- _0 stdout ----
    <anon>:36:9: 36:13 warning: unused variable: `argc`, #[warn(unused_variables)] on by default
<anon>:36 fn main(argc: isize, argv: *const *const u8) -> isize {
                  ^~~~
<anon>:36:22: 36:26 warning: unused variable: `argv`, #[warn(unused_variables)] on by default
<anon>:36 fn main(argc: isize, argv: *const *const u8) -> isize {
                               ^~~~
<anon>:37:9: 37:10 warning: unused variable: `x`, #[warn(unused_variables)] on by default
<anon>:37     let x = box 1;
                  ^
error: linking with `cc` failed: exit code: 1
note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/tmp/rustdoctest.goiITNucvd13/rust_out.0.o" "-o" "/tmp/rustdoctest.goiITNucvd13/rust_out" "-Wl,--gc-sections" "-pie" "-nodefaultlibs" "-L" "/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,-Bstatic" "-Wl,-Bdynamic" "/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-fe3cdf61.rlib" "/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-fe3cdf61.rlib" "-l" "c" "-l" "m" "-l" "rt" "-l" "util" "-l" "compiler-rt"
note: /tmp/rustdoctest.goiITNucvd13/rust_out.0.o: In function `rust_out::main::hec68da2489a63b6a':
rust_out.0.rs:(.text._ZN8rust_out4main17hec68da2489a63b6aE+0x6a): undefined reference to `_Unwind_Resume'
collect2: error: ld returned 1 exit status

error: aborting due to previous error
thread '_0' panicked at 'Box<Any>', src/libsyntax/errors/mod.rs:622
note: Run with `RUST_BACKTRACE=1` for a backtrace.
thread '_0' panicked at 'couldn't compile the test', src/librustdoc/test.rs:281


failures:
    _0

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured

/build/mk/tests.mk:865: recipe for target 'tmp/check-stage2-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-doc-book-lang-items.ok' failed
make: *** [tmp/check-stage2-T-x86_64-unknown-linux-gnu-H-x86_64-unknown-linux-gnu-doc-book-lang-items.ok] Error 101

@alexcrichton Is it possible --enable-orbit doesn't work for everything we run, in this case rustdoc?

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 5, 2016

@alexcrichton The example seems fundamentally broken and only happens to work with old trans because no landing pads get generated for that exact code. Although the fix appears to be simple so I'll just include it here.

Loading

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jun 6, 2016

@eddyb oh we probably don't pass RUSTFLAGS to the rustdoc unit tests (or rustdoc also just doesn't accept -Z), explaining at least why we didn't see it before.

@eddyb also yeah I'm fine with mangling that test or otherwise just ignoring it, it seems like a difficult thing to test anyway. Having it as part of rustdoc also probably isn't helping us much...

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 6, 2016

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 6, 2016

@NastyRigger I have one data point for (compiler) memory usage: 625MB -> 604MB from old trans to MIR trans, on stage1 libsyntax, just after LLVM passes.

Loading

bors added a commit that referenced this issue Jun 7, 2016
[MIR] Fix MIR trans edge cases that showed up on crater.

These fixes cover all of the [regressions found by crater](https://gist.github.com/nikomatsakis/88ce89ed06ef7f7f19bfd1e221d7f7ec) (for #34096).

Two of them were `Pair` edge cases (ZSTs and constants) causing LLVM assertions, the other one was  causing stack overflows in debug scripts compiled in debug mode, due to the `fn_ret_cast` `alloca` ending up in a loop.
@brson
Copy link
Contributor

@brson brson commented Jun 7, 2016

I personally do not see performance regressions as a blocker to enable MIR in nightly by default, because otherwise we will never end up enabling it at al

Once MIR is on the likelyhood of us turning it back on seems very low, so I have to disagree with this. We must hold the line on performance - we have already regressed compile time performance badly this year. Do we have evidence that compile time, run time and code size are on par with oldtrans?

Loading

@MagaTailor
Copy link

@MagaTailor MagaTailor commented Jun 7, 2016

@eddyb @brson I remember comparing binary sizes of parity and rust's lib directory a few weeks ago. I've just finished repeating the tests, and to my utter surprise, instead of 10% larger, MIR produced a 5% smaller parity binary, whereas the lib directory ended up 1.7% smaller. (32-bit ARM data)

During the latter test /usr/bin/time -v make -j2 showed maximum resident memory size of 543124 kB vs 391828 kB (probably librustc), in favour of MIR again. I wasn't able to benchmark compilation times but other than that, MIR seems to have improved immensely!

Edit: Cargo binary is made 8% smaller.

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 7, 2016

@brson Actually, I mentioned libsyntax using less memory to compile, here's the timings/RSS:
Old trans:

time: 11.669; rss: 734MB        translation
  time: 4.534; rss: 621MB       llvm function passes [0]
  time: 94.234; rss: 623MB      llvm module passes [0]
  time: 26.816; rss: 625MB      codegen passes [0]
  time: 0.003; rss: 625MB       codegen passes [0]
time: 126.276; rss: 625MB       LLVM passes

-Z orbit:

time: 12.615; rss: 727MB        translation
  time: 4.449; rss: 634MB       llvm function passes [0]
  time: 69.080; rss: 634MB      llvm module passes [0]
  time: 25.928; rss: 636MB      codegen passes [0]
  time: 0.002; rss: 604MB       codegen passes [0]
time: 100.045; rss: 604MB       LLVM passes

LLVM spends 26 seconds less optimizing the result of trans and uses 20 MBs less of RAM, at the cost of one extra second spent in MIR translation (which could be some missing caching on type/trait stuff).

Loading

@sanxiyn
Copy link
Member

@sanxiyn sanxiyn commented Jun 8, 2016

Will old trans be tested by Buildbot after this PR is merged?

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 8, 2016

@sanxiyn Good question. We could technically flip the MIR bots to be "anti-MIR". cc @alexcrichton

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Jun 27, 2016

@alexcrichton As explained in the PR description, both RUSTFLAGS="-Zorbit=off" and the #[rustc_no_mir] function attribute will work to switch to old trans for the whole compilation or just a function.

Loading

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jun 27, 2016

Clearly I also need a PR to read more things, thanks @eddyb!

Loading

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 19, 2016

I'm going to close this just to help clear out the queue, but the progress for this is tracked on https://github.com/rust-lang/rust/milestone/29

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 1, 2016

After some discussion on IRC, we decided to go forward with this -- MIR is looking good, with no known functional regressions, and we're ready to see how it fares with the broader, nightly audience. =) As @eddyb wrote, albeit for a different nightly:

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

I think that last part may be a bit optimistic, but then... here's hoping. =)

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 1, 2016

@bors r+

Loading

@bors
Copy link
Contributor

@bors bors commented Aug 1, 2016

📌 Commit d2f2df1 has been approved by nikomatsakis

Loading

@bors
Copy link
Contributor

@bors bors commented Aug 1, 2016

Testing commit d2f2df1 with merge 5713448...

Loading

bors added a commit that referenced this issue Aug 1, 2016
Switch to MIR-based translation by default.

This patch makes `-Z orbit` default to "on", which means that by default, functions will be translated from Rust to LLVM IR through the upcoming MIR backend, instead of the antiquated AST backend.

This switch is made possible by the recently merged #33622, #33905 and smaller fixes.

If you experience any issues, please file a report for each of them. You can switch to the old backend to work around problems by either setting `RUSTFLAGS="-Zorbit=off"` or by annotating specific functions with `#[rustc_no_mir]` (which requires `#![feature(rustc_attrs)]` at the crate-level).

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

cc @rust-lang/compiler
@bors
Copy link
Contributor

@bors bors commented Aug 2, 2016

💔 Test failed - auto-mac-32-opt

Loading

@eddyb
Copy link
Member Author

@eddyb eddyb commented Aug 2, 2016

@bors r=nikomatsakis

Loading

@bors
Copy link
Contributor

@bors bors commented Aug 2, 2016

📌 Commit b583711 has been approved by nikomatsakis

Loading

@bors
Copy link
Contributor

@bors bors commented Aug 2, 2016

Testing commit b583711 with merge 34d14e7...

Loading

bors added a commit that referenced this issue Aug 2, 2016
Switch to MIR-based translation by default.

This patch makes `-Z orbit` default to "on", which means that by default, functions will be translated from Rust to LLVM IR through the upcoming MIR backend, instead of the antiquated AST backend.

This switch is made possible by the recently merged #33622, #33905 and smaller fixes.

If you experience any issues, please file a report for each of them. You can switch to the old backend to work around problems by either setting `RUSTFLAGS="-Zorbit=off"` or by annotating specific functions with `#[rustc_no_mir]` (which requires `#![feature(rustc_attrs)]` at the crate-level).

I would like this PR to get into nightly soon so that we can get early feedback in this release cycle and focus on correctness fixes and performance improvements, with the potential for removing the old backend implementation before beta branches off.

cc @rust-lang/compiler
@bors bors merged commit b583711 into rust-lang:master Aug 2, 2016
2 checks passed
Loading
@eddyb eddyb deleted the launch branch Aug 2, 2016
@ernestrc ernestrc mentioned this pull request Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet