Extend MIR inlining to all operand variants #54416

pvdrz · 2018-09-21T01:49:42Z

wesleywiser

Code changes look good to me. Can you also write a test that shows that let f = foo; f() inlines correctly? (See src/test/mir-opt/inline-closure-borrows-arg.rs for an example)

pvdrz · 2018-09-22T20:47:53Z

Of course! give me a couple minutes to do it :)

pvdrz · 2018-09-22T21:09:24Z

Something like this

fn foo() -> i32 {
    let f = |x: i32, y: i32| x + y;
    f(1, -1)
}

would be a good test? Or did I misunderstood something?

wesleywiser · 2018-09-22T21:12:28Z

I think we want something like:

fn foo(x: i32, y: i32) -> i32 {
  x + y
}

fn main() {
  let f = foo;
  println!("{}", f(1, 2));
}

pvdrz · 2018-09-22T21:19:12Z

But then how do I check that the MIR code is in fact what we want? Should I do something like

fn foo(x: i32, y: i32) -> i32 {
  x + y
}

fn bar() -> i32 {
  let f = foo;
  f(1, 2)
}

fn main() {
  println!("{}", bar());
}

And then check that inside bar everything is inlined?

wesleywiser · 2018-09-22T21:24:20Z

Yeah, that looks right to me. Sorry, I don't have a compiler available at the moment so I can't give you the specific MIR instruction but we want to make sure that foo is inlined inside bar which can be done by verifying that the + operation happens inside bar.

pvdrz · 2018-09-22T21:32:40Z

Bad news, I'm getting

fn bar() -> i32{
    let mut _0: i32;
    scope 1 {
    }
    scope 2 {
        let _1: fn(i32, i32) -> i32 {foo};
    }
    let mut _2: fn(i32, i32) -> i32 {foo};
    bb0: {                              
        StorageLive(_1);
        _1 = const foo;
        StorageLive(_2);
        _2 = _1;
        _0 = move _2(const 1i32, const -1i32) -> bb1;
    }
    bb1: {                              
        StorageDead(_2);
        StorageDead(_1);
        return;
    }
    bb2: {
        resume;
    }
}

Which means inlining is not happening.

wesleywiser · 2018-09-22T21:37:12Z

Does adding an #[inline(always)] to foo() make a difference?

pvdrz · 2018-09-22T21:39:24Z

Yeah it does! should I left the #[inline(always)]?

wesleywiser · 2018-09-22T21:41:02Z

Yeah, go ahead and leave it in. The goal is to verify let f = foo; f() can be inlined regardless of whether or not the optimizer actually decides it's a good idea.

pvdrz · 2018-09-22T21:41:52Z

Ok, I'll finish the test and commit again. Any name ideas for this test?

wesleywiser · 2018-09-22T21:43:16Z

I'd either go with inline-any-operand.rs or just plain old issue-54193.rs 🙂

wesleywiser · 2018-09-23T01:53:20Z

Thanks!!

@bors r+

bors · 2018-09-23T01:53:21Z

📌 Commit 99af55b44d9081acf27d998c3413a86188bc600b has been approved by wesleywiser

bors · 2018-09-23T17:49:24Z

⌛ Testing commit 99af55b44d9081acf27d998c3413a86188bc600b with merge 6100d69d31d30131798b14e0b99d9ecb885bd5fc...

bors · 2018-09-23T19:14:19Z

💔 Test failed - status-travis

rust-highfive · 2018-09-23T19:14:21Z

The job x86_64-gnu-nopt of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:21:36] test [mir-opt] mir-opt/lower_128bit_debug_test.rs ... ok
[01:21:36] 
[01:21:36] failures:
[01:21:36] 
[01:21:36] ---- [mir-opt] mir-opt/inline-any-operand.rs stdout ----
[01:21:36] thread '[mir-opt] mir-opt/inline-any-operand.rs' panicked at 'Did not find expected line, error: ran out of mir dump to match against
[01:21:36] Expected Line: "    _0 = Add(move _3, move _4);"
[01:21:36] Test Name: rustc.bar.Inline.after.mir
[01:21:36] ... (elided)
[01:21:36] ... (elided)
[01:21:36] bb0: {
[01:21:36] ... (elided)
[01:21:36] ... (elided)
[01:21:36]     _0 = Add(move _3, move _4);
[01:21:36] ... (elided)
[01:21:36]     return;
[01:21:36] }
[01:21:36] ... (elided)
[01:21:36] Actual:
[01:21:36] fn bar() -> i32{
[01:21:36]     let mut _0: i32;
[01:21:36]     scope 1 {
[01:21:36]         scope 3 {
[01:21:36]     }
[01:21:36]     scope 2 {
[01:21:36]     scope 2 {
[01:21:36]         let _1: fn(i32, i32) -> i32 {foo};
[01:21:36]     }
[01:21:36]     let mut _2: fn(i32, i32) -> i32 {foo};
[01:21:36]     let mut _3: (i32, bool);
[01:21:36]     let mut _4: i32;
[01:21:36]     let mut _5: i32;
[01:21:36]     bb0: {                              
[01:21:36]         StorageLive(_1);
[01:21:36]         _1 = const foo;
[01:21:36]         FakeRead(ForLet, _1);
[01:21:36]         StorageLive(_2);
[01:21:36]         _2 = _1;
[01:21:36]         _4 = const 1i32;
[01:21:36]         _5 = const -1i32;
[01:21:36]         _3 = CheckedAdd(move _4, move _5);
[01:21:36]         assert(!move (_3.1: bool), "attempt to add with overflow") -> bb1;
[01:21:36]     }
[01:21:36]     bb1: {                              
[01:21:36]         _0 = move (_3.0: i32);
[01:21:36]         StorageDead(_2);
[01:21:36]         StorageDead(_1);
[01:21:36]         return;
[01:21:36] }', tools/compiletest/src/runtest.rs:2893:13
[01:21:36] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:21:36] 
[01:21:36] 
---
[01:21:36] 
[01:21:36] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:496:22
[01:21:36] 
[01:21:36] 
[01:21:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/mir-opt" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "mir-opt" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--host-rustcflags" "-Crpath -Zunstable-options " "--target-rustcflags" "-Crpath -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--llvm-version" "8.0.0svn\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:21:36] 
[01:21:36] 
[01:21:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:21:36] Build completed unsuccessfully in 0:17:33
[01:21:36] Build completed unsuccessfully in 0:17:33
[01:21:36] make: *** [check] Error 1
[01:21:36] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0ace4c80
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:3a03c314:start=1537730004150473770,finish=1537730004156518305,duration=6044535
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:06379c15
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:37001098
travis_time:start:37001098
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0aa4c324
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

wesleywiser · 2018-09-24T00:44:52Z

@bors r+

bors · 2018-09-24T00:44:52Z

📌 Commit 1e3c86e has been approved by wesleywiser

@eddyb

Extend MIR inlining to all operand variants This fixes #54193 r? @eddyb

bors · 2018-09-24T02:50:23Z

⌛ Testing commit 1e3c86e with merge 4f1677a...

bors · 2018-09-24T04:32:20Z

💔 Test failed - status-appveyor

pvdrz · 2018-09-24T04:48:53Z

It seems it was a connection problem inside appveyor: https://ci.appveyor.com/project/rust-lang/rust/build/1.0.9089/job/w3c09bbqp6akf7ie#L7208

wesleywiser · 2018-09-24T15:49:58Z

Spurious network issue cc #40474

@bors retry

bors · 2018-09-24T20:07:50Z

⌛ Testing commit 1e3c86e with merge 5c875d9...

@eddyb

Extend MIR inlining to all operand variants This fixes #54193 r? @eddyb

bors · 2018-09-24T22:39:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: wesleywiser
Pushing 5c875d9 to master...

Extend MIR inlining to all operand variants

8efafa1

rust-highfive assigned eddyb Sep 21, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2018

wesleywiser approved these changes Sep 22, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 23, 2018

Add test to check if inlining works for any operand

1e3c86e

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2018

bors added a commit that referenced this pull request Sep 24, 2018

Auto merge of #54416 - christianpoveda:master, r=wesleywiser

4f1677a

Extend MIR inlining to all operand variants This fixes #54193 r? @eddyb

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 24, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2018

bors added a commit that referenced this pull request Sep 24, 2018

Auto merge of #54416 - christianpoveda:master, r=wesleywiser

5c875d9

Extend MIR inlining to all operand variants This fixes #54193 r? @eddyb

bors merged commit 1e3c86e into rust-lang:master Sep 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend MIR inlining to all operand variants #54416

Extend MIR inlining to all operand variants #54416

pvdrz commented Sep 21, 2018

wesleywiser left a comment

pvdrz commented Sep 22, 2018

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

pvdrz commented Sep 22, 2018 •

edited

Loading

wesleywiser commented Sep 22, 2018 •

edited

Loading

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

wesleywiser commented Sep 23, 2018

bors commented Sep 23, 2018

bors commented Sep 23, 2018

bors commented Sep 23, 2018

rust-highfive commented Sep 23, 2018

wesleywiser commented Sep 24, 2018

bors commented Sep 24, 2018

bors commented Sep 24, 2018

bors commented Sep 24, 2018

pvdrz commented Sep 24, 2018

wesleywiser commented Sep 24, 2018

bors commented Sep 24, 2018

bors commented Sep 24, 2018

Extend MIR inlining to all operand variants #54416

Extend MIR inlining to all operand variants #54416

Conversation

pvdrz commented Sep 21, 2018

wesleywiser left a comment

Choose a reason for hiding this comment

pvdrz commented Sep 22, 2018

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

pvdrz commented Sep 22, 2018 • edited Loading

wesleywiser commented Sep 22, 2018 • edited Loading

pvdrz commented Sep 22, 2018

wesleywiser commented Sep 22, 2018

wesleywiser commented Sep 23, 2018

bors commented Sep 23, 2018

bors commented Sep 23, 2018

bors commented Sep 23, 2018

rust-highfive commented Sep 23, 2018

wesleywiser commented Sep 24, 2018

bors commented Sep 24, 2018

bors commented Sep 24, 2018

bors commented Sep 24, 2018

pvdrz commented Sep 24, 2018

wesleywiser commented Sep 24, 2018

bors commented Sep 24, 2018

bors commented Sep 24, 2018

pvdrz commented Sep 22, 2018 •

edited

Loading

wesleywiser commented Sep 22, 2018 •

edited

Loading