Stream: t-compiler

Topic: turning on MIR inlining


Wesley Wiser (Oct 10 2018 at 15:33, on Zulip):

Hi @nikomatsakis, per our conversation on Discord, I've been trying to resurrect PR #48602. It looks to me like @eddyb was trying to get the code to build "enough" to get a perf run and see what the potential performance improvements might be. I've worked around a few more issues but I'm still getting ICEs when trying to build rustdoc.

Should I keep trying to just get a build out so we can measure the performance or should I try a more principled approach and try to actually fix the bugs rather than work around them?

Also, what's the best way to troubleshoot a broken stage2 compiler that ICEs? The stage1 compiler seems to work fine and can build rustdoc but the stage2 compiler ICEs when building it.

nikomatsakis (Oct 10 2018 at 15:33, on Zulip):

I'd be inclined to fix the bugs first ;)

nikomatsakis (Oct 10 2018 at 15:33, on Zulip):

though stage2 only bugs are definitely the worst kind

nikomatsakis (Oct 10 2018 at 15:35, on Zulip):

what is the ICE? I don't really know a "magic bullet" to troubleshoot beyond trying to track down where it is failing; a stage2 error however could be the result of the stage1 compiler miscompiling the compiler itself, so the point where it fails might not itself be broken per se (e.g., maybe it is generating bad code due to inlining)

nikomatsakis (Oct 10 2018 at 15:35, on Zulip):

given that hypothesis, one way to track down where the problem is would be to build on the "optimization fuel" stuff we have

nikomatsakis (Oct 10 2018 at 15:35, on Zulip):

basically it would let you perform only the first N inlining attempts

nikomatsakis (Oct 10 2018 at 15:35, on Zulip):

you can then bisect around with N to try and find which inlining goes wrong

Wesley Wiser (Oct 10 2018 at 15:36, on Zulip):

Uh, I don't have it handy unfortunately but it's a generic "you called .unwrap() on a None" type error.

Wesley Wiser (Oct 10 2018 at 15:36, on Zulip):

I think I remember the call site... let me see if I can find it

nikomatsakis (Oct 10 2018 at 15:38, on Zulip):

sounds like the backtrace could be handy

Wesley Wiser (Oct 10 2018 at 15:42, on Zulip):

Hmm. I can't seem to find it offhand.

I'll post it tonight when I get back to my pc

Wesley Wiser (Oct 10 2018 at 15:43, on Zulip):

I think there's invalid inlining happening.

Wesley Wiser (Oct 10 2018 at 15:43, on Zulip):

I worked around another ICE by adding a #[inline(never)] to a trait method with a default body.

nikomatsakis (Oct 10 2018 at 15:45, on Zulip):

@Wesley Wiser well that does give a pretty good clue then as to where the bugs are ;)

nikomatsakis (Oct 10 2018 at 15:45, on Zulip):

maybe worth trying to fix that one that you worked around first

nikomatsakis (Oct 10 2018 at 15:45, on Zulip):

in hopes that there is one underlying bug

Wesley Wiser (Oct 10 2018 at 15:46, on Zulip):

I guess I can dump the MIR by manually invoking the compiler for the appropriate crate and then passing -Z dump-mir= or whatever?

Wesley Wiser (Oct 10 2018 at 15:47, on Zulip):

To validate that it is a bad optimization

nikomatsakis (Oct 10 2018 at 15:51, on Zulip):

yes, you can do that

nikomatsakis (Oct 10 2018 at 15:51, on Zulip):

it is kind of a pain to invoke the compiler during the bootstrap cycle sometimes

nikomatsakis (Oct 10 2018 at 15:51, on Zulip):

due to the extensive (and, to me, irritating) use of env vars

nikomatsakis (Oct 10 2018 at 15:52, on Zulip):

I usually do x.py build -vv

nikomatsakis (Oct 10 2018 at 15:52, on Zulip):

right before the failing compile there is something like rustc command: ... followed by a super long line

nikomatsakis (Oct 10 2018 at 15:52, on Zulip):

sometimes when you copy-and-paste that and run it from the src directory, it works

nikomatsakis (Oct 10 2018 at 15:52, on Zulip):

other times...it doesn't

nikomatsakis (Oct 10 2018 at 15:52, on Zulip):

if there's a better way, I'd like to know it :)

nikomatsakis (Oct 10 2018 at 15:53, on Zulip):

but you can use RUSTFLAGS=-Zdump-mir=... etc and run x.py, too

Wesley Wiser (Oct 10 2018 at 15:53, on Zulip):

Ah, interesting. I always forget about that. Thanks!

Wesley Wiser (Oct 10 2018 at 15:54, on Zulip):

I guess if it's the issue I think is, I could probably just write a test that repro's the issue.

Wesley Wiser (Oct 10 2018 at 23:48, on Zulip):

@nikomatsakis The stack trace is:

Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Building rustdoc for stage2 (x86_64-unknown-linux-gnu)
   Compiling libc v0.2.36
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', libcore/option.rs:335:21
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:206
   3: std::panicking::default_hook
             at libstd/panicking.rs:222
   4: core::ops::function::Fn::call
             at librustc/util/common.rs:50
             at /home/wesley/code/rust/rust/src/libcore/ops/function.rs:73
   5: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:401
   6: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:347
   7: rust_begin_unwind
             at libstd/panicking.rs:323
   8: core::panicking::panic_fmt
             at libcore/panicking.rs:71
   9: core::panicking::panic
             at libcore/panicking.rs:51
  10: syntax::ext::expand::MacroExpander::expand
             at /home/wesley/code/rust/rust/src/libcore/macros.rs:20
             at libsyntax/ext/expand.rs:307
  11: syntax::ext::expand::MacroExpander::expand_crate
             at libsyntax/ext/expand.rs:245
  12: rustc_driver::driver::phase_2_configure_and_expand_inner::{{closure}}
             at librustc_driver/driver.rs:795
  13: rustc::util::common::time
             at /home/wesley/code/rust/rust/src/librustc/util/common.rs:141
  14: rustc_driver::driver::phase_2_configure_and_expand
             at librustc_driver/driver.rs:754
             at librustc_driver/driver.rs:606
  15: rustc_driver::driver::compile_input
             at librustc_driver/driver.rs:129
  16: rustc_driver::run_compiler
             at librustc_driver/lib.rs:508

error: internal compiler error: unexpected panic
Wesley Wiser (Oct 11 2018 at 03:47, on Zulip):

There's definitely misoptimization going on. The following program prints 1 instead of 2 with MIR inlining turned on

fn main() {
    println!("{}", test(&()));
}

fn test(x: &X) -> u32 {
    x.y()
}

trait X {
    fn y(&self) -> u32 {
        1
    }
}

impl X for () {
    fn y(&self) -> u32 {
        2
    }
}

I'm working on a fix for that.

nagisa (Oct 11 2018 at 13:23, on Zulip):

I do not remember being very pleased with the current state that our inliner is, even if I didn’t considered it to be buggy. It feels more like a PoC to me.

Wesley Wiser (Oct 16 2018 at 16:01, on Zulip):

Just to give a small status update: I'm still working on this. I pushed a fix for the virtual call inlining issue (#55046) which has been merged. I'm now investigating another miscompilation which results in a very confounding backtrace:

[continues for another 209539 stack frames]
#209540 0x0000555555563236 in core::ptr::<impl core::cmp::PartialOrd for *const T>::partial_cmp ()
#209541 0x00005555555631a7 in core::ptr::<impl core::cmp::Ord for *const T>::cmp ()
#209542 0x0000555555563236 in core::ptr::<impl core::cmp::PartialOrd for *const T>::partial_cmp ()
#209543 0x00005555555631a7 in core::ptr::<impl core::cmp::Ord for *const T>::cmp ()
#209544 0x0000555555563236 in core::ptr::<impl core::cmp::PartialOrd for *const T>::partial_cmp ()
#209545 0x00005555555631a7 in core::ptr::<impl core::cmp::Ord for *const T>::cmp ()
#209546 0x0000555555564b4b in <[A] as core::slice::SliceOrd<A>>::compare ()
#209547 0x0000555555564a26 in core::slice::<impl core::cmp::Ord for [T]>::cmp ()
#209548 0x00005555555620be in core::array::<impl core::cmp::Ord for [T; <unevaluated[]>]>::cmp ()
#209549 0x0000555555561624 in raw_fat_ptr::main::{{closure}} ()
#209550 0x000055555556051d in alloc::slice::<impl [T]>::sort_by::{{closure}} ()
#209551 0x0000555555560028 in alloc::slice::insert_head ()
#209552 0x000055555555f0dd in alloc::slice::merge_sort ()
#209553 0x00005555555615b1 in raw_fat_ptr::main ()
#209554 0x0000555555562123 in std::rt::lang_start::{{closure}} ()
#209555 0x0000555555566168 in std::rt::lang_start_internal::{{closure}} () at libstd/rt.rs:59
#209556 std::panicking::try::do_call (data=0x7fffffffdd48 "\240\335\377\377\377\177\000") at libstd/panicking.rs:305
#209557 0x000055555557b59f in __rust_maybe_catch_panic (f=0x0, data=0x7fffffffd758 "\207\334\377\377\377\177\000", data_ptr=0x7fffffffdd58, vtable_ptr=0x7fffffffdd50) at libpanic_unwind/lib.rs:102
#209558 0x000055555556605a in std::panicking::try (f=...) at libstd/panicking.rs:284
#209559 0x000055555556efc0 in std::rt::lang_start_internal (main=..., argc=1, argv=0x7fffffffdf48) at libstd/panic.rs:361
#209560 0x0000555555562107 in std::rt::lang_start ()
#209561 0x000055555556165e in main ()
nikomatsakis (Oct 16 2018 at 16:30, on Zulip):

@Wesley Wiser nice work on #55046!

Wesley Wiser (Oct 16 2018 at 16:50, on Zulip):

@nikomatsakis Thanks!

Wesley Wiser (Oct 16 2018 at 16:50, on Zulip):

BTW, are you going to be at RBR this week?

nikomatsakis (Oct 16 2018 at 16:51, on Zulip):

yes

nikomatsakis (Oct 16 2018 at 16:51, on Zulip):

I arrive Thu night

Wesley Wiser (Oct 16 2018 at 16:52, on Zulip):

Cool! Looking forward to meeting you :)

RalfJ (Oct 17 2018 at 09:50, on Zulip):

@Wesley Wiser might be worth also fixing https://github.com/rust-lang/rust/issues/50411 before enabling more MIR optimziations per default

RalfJ (Oct 17 2018 at 09:50, on Zulip):

and https://github.com/rust-lang/rust/issues/52691 might also still be around

Wesley Wiser (Oct 17 2018 at 14:14, on Zulip):

@RalfJ Good idea! I'll definitely take a look into that. Right now I'm just trying to the compiler unbroken enough to see what the potential performance differences might be. I'm sure there's a lot of issues to solve before we can turn it on everywhere by default.

RalfJ (Oct 17 2018 at 14:20, on Zulip):

@Wesley Wiser okay. :) Great to see someone push this! miri has been sitting on these bugs since about forever.

Wesley Wiser (Nov 13 2018 at 20:35, on Zulip):

Status update: Still working on this :) Since the last update, I implemented optimization fuel for the inliner (#55739 thanks for the idea @nikomatsakis!), fixed the optimization fuel code so it doesn't break Rustbuild (#55495 thanks @pnkfelix for the assist with the test suite!), and found the other place we were inlining virtual function calls (#55802).

At this point, with a small patch to always run the MIR inliner, the compiler will bootstrap successfully to stage 2 but there's quite a few test failures.
In just the run-pass suite there are these failures:

    [run-pass] run-pass/associated-consts/associated-const-cross-crate-defaults.rs
    [run-pass] run-pass/associated-consts/associated-const-use-default.rs
    [run-pass] run-pass/backtrace-debuginfo.rs
    [run-pass] run-pass/cross-crate/xcrate_generic_fn_nested_return.rs
    [run-pass] run-pass/debuginfo-lto.rs
    [run-pass] run-pass/fat-lto.rs
    [run-pass] run-pass/generator/smoke.rs
    [run-pass] run-pass/issues/issue-11205.rs
    [run-pass] run-pass/lto-many-codegen-units.rs
    [run-pass] run-pass/lto-still-runs-thread-dtors.rs
    [run-pass] run-pass/optimization-fuel-0.rs
    [run-pass] run-pass/optimization-fuel-1.rs
    [run-pass] run-pass/panic-runtime/lto-abort.rs
    [run-pass] run-pass/panic-runtime/lto-unwind.rs
    [run-pass] run-pass/rfcs/rfc-2005-default-binding-mode/constref.rs
    [run-pass] run-pass/sepcomp/sepcomp-lib-lto.rs
    [run-pass] run-pass/stack-probes-lto.rs
    [run-pass] run-pass/ufcs-polymorphic-paths.rs

I'm currently trying to track down the next optimization issue (perhaps something to do with LTO?) by disabling inlining for a specific crate using the fuel flag (RUSTFLAGS="-Z fuel={lib}=0" ./x.py test --stage 1) but so far I've been unable to find the issue.

If anyone has any ideas, please feel free to leave a comment! Otherwise, I'll continue my search through the rustc crates looking for the culprit.

nikomatsakis (Nov 13 2018 at 21:58, on Zulip):

@Wesley Wiser wow, nice

nikomatsakis (Nov 13 2018 at 21:58, on Zulip):

which test are you trying to fix?

nikomatsakis (Nov 13 2018 at 21:58, on Zulip):

those are all... interesting tests, they're the "scary ones" from my POV :)

nikomatsakis (Nov 13 2018 at 21:59, on Zulip):

scary ones being cross-crate etc

nikomatsakis (Nov 13 2018 at 21:59, on Zulip):

maybe run-pass/ufcs-polymorphic-paths.rs is not so bad

nikomatsakis (Nov 13 2018 at 21:59, on Zulip):

that's probably the one I would start with...

Wesley Wiser (Nov 13 2018 at 22:18, on Zulip):

@nikomatsakis Any of them :smile: I've tried turning fuel off for a bunch of different crates but the same tests keep failing. (I've been tracking some notes here https://github.com/wesleywiser/rust/issues/2)

Last update: Nov 22 2019 at 05:30UTC