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

Improve MIR match generation for ranges #56810

Merged
merged 6 commits into from Dec 17, 2018

Conversation

Projects
None yet
8 participants
@sinkuu
Copy link
Contributor

sinkuu commented Dec 14, 2018

Improves MIR match generation to rule out ranges/values distinct from the range that has been tested. e.g., for this code:

match x {
    0..=5 if b => 0,
    6..=10 => 1,
    _ => 2,
}

MIR (before):

bb0: { ...; _4 = Le(const 0i32, _1); switchInt(move _4) -> [false: bb6, otherwise: bb5]; }
bb1: { _3 = const 0i32; goto -> bb8; }
bb2: { _6 = _2; switchInt(move _6) -> [false: bb6, otherwise: bb1]; } // If `!b`, jumps to test if `6 <= x <= 10`.
bb3: { _3 = const 1i32; goto -> bb8; }
bb4: { _3 = const 2i32; goto -> bb8; }
bb5: { _5 = Le(_1, const 5i32); switchInt(move _5) -> [false: bb6, otherwise: bb2]; }
bb6: { _7 = Le(const 6i32, _1); switchInt(move _7) -> [false: bb4, otherwise: bb7]; }
bb7: { _8 = Le(_1, const 10i32); switchInt(move _8) -> [false: bb4, otherwise: bb3]; }

MIR (after):

bb0: { ...; _4 = Le(const 0i32, _1); switchInt(move _4) -> [false: bb5, otherwise: bb6]; }
bb1: { _3 = const 0i32; goto -> bb8; }
bb2: { _6 = _2; switchInt(move _6) -> [false: bb4, otherwise: bb1]; } // If `!b`, jumps to `_ =>` arm.
bb3: { _3 = const 1i32; goto -> bb8; }
bb4: { _3 = const 2i32; goto -> bb8; }
bb5: { _7 = Le(const 6i32, _1); switchInt(move _7) -> [false: bb4, otherwise: bb7]; }
bb6: { _5 = Le(_1, const 5i32); switchInt(move _5) -> [false: bb5, otherwise: bb2]; }
bb7: { _8 = Le(_1, const 10i32); switchInt(move _8) -> [false: bb4, otherwise: bb3]; }

cc #29623

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 14, 2018

r? @cramertj

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

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 14, 2018

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.

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)

@sinkuu sinkuu reopened this Dec 14, 2018

@sinkuu sinkuu force-pushed the sinkuu:build_match branch from 8e25aaf to 27c8864 Dec 14, 2018

@sinkuu sinkuu changed the title Improve match MIR generation for ranges Improve MIR match generation for ranges Dec 14, 2018

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 14, 2018

The job x86_64-gnu-llvm-5.0 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.
travis_time:end:108a7d7f:start=1544782750807300535,finish=1544782815454226521,duration=64646925986
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:20:04] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:20:04] expected success, got: exit code: 101
[00:20:04] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:20:04] Build completed unsuccessfully in 0:16:45
[00:20:04] Makefile:28: recipe for target 'all' failed
[00:20:04] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:102ed5b2
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Dec 14 10:40:28 UTC 2018

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)

@sinkuu sinkuu closed this Dec 14, 2018

@sinkuu sinkuu reopened this Dec 14, 2018

@sinkuu sinkuu force-pushed the sinkuu:build_match branch from 27c8864 to 856e3b1 Dec 14, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 14, 2018

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

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 14, 2018

sinkuu added some commits Dec 13, 2018

Improve `match` MIR generation for ranges
Makes testing a range rule out ranges/constant
covered by the range that is being tested

@sinkuu sinkuu force-pushed the sinkuu:build_match branch from 856e3b1 to 9daa823 Dec 14, 2018

@matthewjasper

This comment has been minimized.

Copy link
Contributor

matthewjasper commented Dec 15, 2018

Do we generate a false edge from the !b path to the test for 6..=10 now?

@sinkuu

This comment has been minimized.

Copy link
Contributor

sinkuu commented Dec 15, 2018

Imaginary part of a false edge always points to Candidate.next_candidate_pre_binding_block, which is populated in Builder::match_expr, so it won't be affected.

Essential* diff of SimplifyBranch-initial.before for that example:

@@ -58,7 +58,7 @@
         switchInt(move _6) -> [false: bb10, otherwise: bb1];
     }
     bb10: {
-        falseEdges -> [real: bb11, imaginary: bb5];
+        falseEdges -> [real: bb6, imaginary: bb5];
     }
     bb11: {
         _7 = Le(const 6i32, _1);

(* this PR changes the order of BBs, so I swapped a few of them to reduce amount of diff)

@oli-obk
Copy link
Contributor

oli-obk left a comment

I think it would be a good idea to add a mir-opt test ensuring that we don't regress this in the future, but these tests are such a pain to keep updated that we should punt this to the future.

This is probably not going to affect the perf tests as it's unlikely that we have any doing such range matches in a perf critical path. Have you done any benchmarks locally? Just curious, does not affect this PR at all.

Show resolved Hide resolved src/librustc_mir/build/matches/test.rs Outdated
Show resolved Hide resolved src/librustc_mir/build/matches/test.rs Outdated
Show resolved Hide resolved src/librustc_mir/build/matches/test.rs Outdated
return true;
}

let no_overlap = (|| {

This comment has been minimized.

@oli-obk

oli-obk Dec 15, 2018

Contributor

we can use the try syntax inside the compiler.

This comment has been minimized.

@sinkuu

sinkuu Dec 15, 2018

Contributor

try syntax seems to require not only #![feature(try_block)] but also --edition 2018 for try keyword.

@sinkuu

This comment has been minimized.

Copy link
Contributor

sinkuu commented Dec 15, 2018

Have you done any benchmarks locally?

No.

@sinkuu sinkuu force-pushed the sinkuu:build_match branch from 95594ae to 0aab437 Dec 17, 2018

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 17, 2018

The job x86_64-gnu-llvm-6.0 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.
travis_time:end:19fde8d3:start=1545033388039394459,finish=1545033444296127243,duration=56256732784
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:03:08] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:08] tidy error: /checkout/src/test/mir-opt/match_test.rs: missing trailing newline
[00:03:10] some tidy checks failed
[00:03:10] 
[00:03:10] 
[00:03:10] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:10] 
[00:03:10] 
[00:03:10] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:10] Build completed unsuccessfully in 0:00:49
[00:03:10] Build completed unsuccessfully in 0:00:49
[00:03:10] make: *** [tidy] Error 1
[00:03:10] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1ca91566
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon Dec 17 08:00:43 UTC 2018
---
travis_time:end:05f89200:start=1545033643754175432,finish=1545033643760154801,duration=5979369
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0a5425a0
$ 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:098aab04
travis_time:start:098aab04
$ 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:22d5eff3
$ 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)

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 17, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

⌛️ Trying commit d66a55e with merge f8b8ae3...

bors added a commit that referenced this pull request Dec 17, 2018

Auto merge of #56810 - sinkuu:build_match, r=<try>
Improve MIR match generation for ranges

Improves MIR match generation to rule out ranges/values distinct from the range that has been tested. e.g., for this code:

```rust
match x {
    0..=5 if b => 0,
    6..=10 => 1,
    _ => 2,
}
```

MIR (before):

```rust
bb0: { ...; _4 = Le(const 0i32, _1); switchInt(move _4) -> [false: bb6, otherwise: bb5]; }
bb1: { _3 = const 0i32; goto -> bb8; }
bb2: { _6 = _2; switchInt(move _6) -> [false: bb6, otherwise: bb1]; } // If `!b`, jumps to test if `6 <= x <= 10`.
bb3: { _3 = const 1i32; goto -> bb8; }
bb4: { _3 = const 2i32; goto -> bb8; }
bb5: { _5 = Le(_1, const 5i32); switchInt(move _5) -> [false: bb6, otherwise: bb2]; }
bb6: { _7 = Le(const 6i32, _1); switchInt(move _7) -> [false: bb4, otherwise: bb7]; }
bb7: { _8 = Le(_1, const 10i32); switchInt(move _8) -> [false: bb4, otherwise: bb3]; }
```

MIR (after):
```rust
bb0: { ...; _4 = Le(const 0i32, _1); switchInt(move _4) -> [false: bb5, otherwise: bb6]; }
bb1: { _3 = const 0i32; goto -> bb8; }
bb2: { _6 = _2; switchInt(move _6) -> [false: bb4, otherwise: bb1]; } // If `!b`, jumps to `_ =>` arm.
bb3: { _3 = const 1i32; goto -> bb8; }
bb4: { _3 = const 2i32; goto -> bb8; }
bb5: { _7 = Le(const 6i32, _1); switchInt(move _7) -> [false: bb4, otherwise: bb7]; }
bb6: { _5 = Le(_1, const 5i32); switchInt(move _5) -> [false: bb5, otherwise: bb2]; }
bb7: { _8 = Le(_1, const 10i32); switchInt(move _8) -> [false: bb4, otherwise: bb3]; }
```

cc #29623
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 17, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 17, 2018

Success: Queued f8b8ae3 with parent 63f8e6e, comparison URL.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

☀️ Test successful - status-travis
State: approved= try=True

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 17, 2018

Finished benchmarking try commit f8b8ae3

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 17, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

📌 Commit d66a55e has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

⌛️ Testing commit d66a55e with merge 54f3cd6...

bors added a commit that referenced this pull request Dec 17, 2018

Auto merge of #56810 - sinkuu:build_match, r=oli-obk
Improve MIR match generation for ranges

Improves MIR match generation to rule out ranges/values distinct from the range that has been tested. e.g., for this code:

```rust
match x {
    0..=5 if b => 0,
    6..=10 => 1,
    _ => 2,
}
```

MIR (before):

```rust
bb0: { ...; _4 = Le(const 0i32, _1); switchInt(move _4) -> [false: bb6, otherwise: bb5]; }
bb1: { _3 = const 0i32; goto -> bb8; }
bb2: { _6 = _2; switchInt(move _6) -> [false: bb6, otherwise: bb1]; } // If `!b`, jumps to test if `6 <= x <= 10`.
bb3: { _3 = const 1i32; goto -> bb8; }
bb4: { _3 = const 2i32; goto -> bb8; }
bb5: { _5 = Le(_1, const 5i32); switchInt(move _5) -> [false: bb6, otherwise: bb2]; }
bb6: { _7 = Le(const 6i32, _1); switchInt(move _7) -> [false: bb4, otherwise: bb7]; }
bb7: { _8 = Le(_1, const 10i32); switchInt(move _8) -> [false: bb4, otherwise: bb3]; }
```

MIR (after):
```rust
bb0: { ...; _4 = Le(const 0i32, _1); switchInt(move _4) -> [false: bb5, otherwise: bb6]; }
bb1: { _3 = const 0i32; goto -> bb8; }
bb2: { _6 = _2; switchInt(move _6) -> [false: bb4, otherwise: bb1]; } // If `!b`, jumps to `_ =>` arm.
bb3: { _3 = const 1i32; goto -> bb8; }
bb4: { _3 = const 2i32; goto -> bb8; }
bb5: { _7 = Le(const 6i32, _1); switchInt(move _7) -> [false: bb4, otherwise: bb7]; }
bb6: { _5 = Le(_1, const 5i32); switchInt(move _5) -> [false: bb5, otherwise: bb2]; }
bb7: { _8 = Le(_1, const 10i32); switchInt(move _8) -> [false: bb4, otherwise: bb3]; }
```

cc #29623
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 54f3cd6 to master...

@bors bors merged commit d66a55e into rust-lang:master Dec 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment