Optimize slice.{r}position result bounds check by arthurprs · Pull Request #45501 · rust-lang/rust

arthurprs · 2017-10-24T20:31:32Z

All the unrolling makes it hard for llvm to optimize it out.

Sample https://godbolt.org/g/R9nLvC

rust-highfive · 2017-10-24T20:31:42Z

r? @dtolnay

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

dtolnay

Nicely done!

dtolnay · 2017-10-25T01:36:19Z

@bors r+

bors · 2017-10-25T01:36:19Z

📌 Commit b9760de has been approved by dtolnay

dtolnay · 2017-10-25T01:37:29Z

Travis is showing:

[00:58:51] error[E0599]: no method named `position` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:20:15
[00:58:51] |
[00:58:51] 20 | assert!(b.position(|&v| v == 9) == None);
[00:58:51] | ^^^^^^^^
[00:58:51]
[00:58:51] error[E0599]: no method named `position` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:21:15
[00:58:51] |
[00:58:51] 21 | assert!(b.position(|&v| v == 5) == Some(3));
[00:58:51] | ^^^^^^^^
[00:58:51]
[00:58:51] error[E0599]: no method named `position` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:22:15
[00:58:51] |
[00:58:51] 22 | assert!(b.position(|&v| v == 3) == Some(2));
[00:58:51] | ^^^^^^^^
[00:58:51]
[00:58:51] error[E0599]: no method named `position` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:23:15
[00:58:51] |
[00:58:51] 23 | assert!(b.position(|&v| v == 0) == None);
[00:58:51] | ^^^^^^^^
[00:58:51]
[00:58:51] error[E0599]: no method named `rposition` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:29:15
[00:58:51] |
[00:58:51] 29 | assert!(b.rposition(|&v| v == 9) == None);
[00:58:51] | ^^^^^^^^^
[00:58:51]
[00:58:51] error[E0599]: no method named `rposition` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:30:15
[00:58:51] |
[00:58:51] 30 | assert!(b.rposition(|&v| v == 5) == Some(4));
[00:58:51] | ^^^^^^^^^
[00:58:51]
[00:58:51] error[E0599]: no method named `rposition` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:31:15
[00:58:51] |
[00:58:51] 31 | assert!(b.rposition(|&v| v == 3) == Some(2));
[00:58:51] | ^^^^^^^^^
[00:58:51]
[00:58:51] error[E0599]: no method named `rposition` found for type `[{integer}; 5]` in the current scope
[00:58:51] --> /checkout/src/libcore/../libcore/tests/slice.rs:32:15
[00:58:51] |
[00:58:51] 32 | assert!(b.rposition(|&v| v == 0) == None);
[00:58:51] | ^^^^^^^^^

@bors r-

arthurprs · 2017-10-25T11:09:54Z

Sorry about that, broken tests... They're fixed now.

kennytm · 2017-10-25T13:44:38Z

@bors r=dtolnay

bors · 2017-10-25T13:44:39Z

📌 Commit 3529709 has been approved by dtolnay

bors · 2017-10-26T13:04:45Z

⌛ Testing commit 35297095934dd2d0235a61af4dd91f3b304a9b55 with merge 916521726c4b54b3cfe96122e2c98d2b735f819d...

bors · 2017-10-26T14:14:13Z

💔 Test failed - status-travis

kennytm · 2017-10-26T14:24:32Z

@bors retry

dist-x86_64-musl, failed to test stage2-libstd, #44899 ...?

Testing libstd stage2 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-musl)
[01:00:44] Compiling rand v0.0.0 (file:///checkout/src/librand)
[01:00:44] Compiling alloc v0.0.0 (file:///checkout/src/liballoc)
[01:00:44] Compiling std_unicode v0.0.0 (file:///checkout/src/libstd_unicode)
[01:00:44] Compiling core v0.0.0 (file:///checkout/src/libcore)
[01:00:53] Compiling std v0.0.0 (file:///checkout/src/libstd)
[01:01:35] rustc: /checkout/src/llvm/lib/Analysis/ValueTracking.cpp:1594: void computeKnownBits(const llvm::Value*, llvm::APInt&, llvm::APInt&, unsigned int, const {anonymous}::Query&): Assertion `(KnownZero & KnownOne) == 0 && "Bits known to be one AND zero?"' failed.
[01:01:35] error: Could not compile `alloc`.
[01:01:35] warning: build failed, waiting for other jobs to finish...
[01:03:14] error: build failed

bors · 2017-10-26T16:58:31Z

⌛ Testing commit 35297095934dd2d0235a61af4dd91f3b304a9b55 with merge 18d66a16d23811fffa2aefad15122c676b7b7f23...

bors · 2017-10-26T18:16:12Z

💔 Test failed - status-travis

kennytm · 2017-10-26T19:42:22Z

Same error. It's legit then.

@arthurprs Could you figure out what triggers #44899 here, and workaround it?

arthurprs · 2017-10-26T21:01:44Z

I moved the assume to the end. Can we try again?

bluss · 2017-10-26T21:04:47Z

self.len seems wrong here.

Thanks. I ran out of caffeine ☕

arthurprs · 2017-10-29T17:17:08Z

Ok, this is definitely not safe.

bluss · 2017-10-29T17:22:39Z

(This one should be before += 1 to be correct.)

Good point. Moving it before the mutation probably kills the optimization though.

@arthurprs What if you make it assume(0 < index && index <= len)

We can try that too. Lets try a few more things.

bluss · 2017-10-29T17:23:55Z

A bit of a tangential thought, maybe we should have a version of assume that's like debug_assert in debug builds and assume in release builds. I'm a proponent of debug checking almost all unsafe code.

scottmcm · 2017-10-29T23:08:22Z

Is there a way to have a test for this? I have a PR that will conflict massively with this, and selfishly I'd like an easy way to ensure I don't regress it by accident.

Another thought: is this something that should be done for everything that's TrustedLen?

arthurprs · 2017-10-30T07:41:41Z

@kennytm is it possible to do a bors run but not merge this?

@scottmcm we're still trying to make this work🤔. As with most optimizations this is hard to test for regressions.

kennytm · 2017-10-30T09:09:52Z

@arthurprs Edit the .travis.yml and comment out any of the if: branch = auto line to test on the CI.

arthurprs · 2017-10-30T14:28:37Z

It failed in both musl builds :sad: Any ideas?

assume is probably dangerous across function boundaries.

arthurprs · 2017-11-01T08:29:13Z

I'm gonna wait for #45595 to get in so I can try more combinations. I'm not felling hopeful though 😞

kennytm · 2017-11-01T08:47:01Z

@arthurprs Have you tried to reproduce locally via Docker? Just run

./src/ci/docker/run.sh dist-x86_64-musl

arthurprs · 2017-11-01T10:30:16Z

I gave up trying, I keep getting

 ---> bec314029591
Successfully built bec314029591
Successfully tagged rust-ci:latest
configure: processing command line
configure: configure: rust.dist-src := False
configure: build.extended := True
configure: build.target := ['x86_64-unknown-linux-musl']
configure: build.submodules := False
configure: rust.debug-assertions := True
configure: llvm.assertions := True
configure: build.locked-deps := True
configure: llvm.ccache := sccache
configure: build.openssl-static := True
configure: target.x86_64-unknown-linux-musl.musl-root := /musl-x86_64
configure: build.configure-args := ['--target=x86_64-unknown-linux-musl', '--musl ...
configure: configure: writing `config.toml` in current directory
Traceback (most recent call last):
File "/checkout/src/bootstrap/configure.py", line 422, in <module>
with open('Makefile', 'w') as f:
IOError: [Errno 13] Permission denied: 'Makefile

dtolnay · 2017-11-13T17:24:19Z

I am closing the PR because it doesn't look like it is being worked on. I filed #45964 to follow up and try again. It seems like assume(index < len) should have worked!

…k, r=dtolnay Optimize slice.{r}position result bounds check Second attempt of rust-lang#45501Fixesrust-lang#45964 Demo: https://godbolt.org/g/N4mBHp

Use the slice length to hint the optimizer about iter.position result Using the len of the iterator doesn't give the same result. That's also why we can't generalize it to all TrustedLen iterators. Problem demo: https://godbolt.org/g/MXg2ae Fix demo: https://godbolt.org/g/P8q5aZ Second attempt of #47333 Third attempt of #45501Fixes#45964

@eddyb

Try to fix 48116 and 48192 The bug #48116 happens because of a misoptimization of the `import_path_to_string` function, where a `names` slice is empty but the `!names.is_empty()` branch is executed. https://github.com/rust-lang/rust/blob/4d2d3fc5dadf894a8ad709a5860a549f2c0b1032/src/librustc_resolve/resolve_imports.rs#L1015-L1042 Yesterday, @eddyb had locally reproduced the bug, and [came across the `position` function](https://mozilla.logbot.info/rust-infra/20180214#c14296834) where the `assume()` call is found to be suspicious. We have *not* concluded that this `assume()` causes #48116, but given [the reputation of `assume()`](#45501 (comment)), this seems higher relevant. Here we try to see if commenting it out can fix the errors. Later @alexcrichton has bisected and found a potential bug [in the LLVM side](#48116 (comment)). We are currently testing if reverting that LLVM commit is enough to stop the bug. If true, this PR can be reverted (keep the `assume()`) and we could backport the LLVM patch instead. (This PR also includes an earlier commit from #48127 for help debugging ICE happening in compile-fail/parse-fail tests.) The PR also reverts #48059, which seems to cause #48192. r? @alexcrichton cc @eddyb, @arthurprs (#47333)

rust-highfive assigned dtolnay Oct 24, 2017

arthurprs force-pushed the iter-position-bounds-check branch 2 times, most recently from 3a32678 to b9760deCompare October 24, 2017 21:05

dtolnay approved these changes Oct 25, 2017
View reviewed changes

arthurprs force-pushed the iter-position-bounds-check branch from b9760de to 3529709Compare October 25, 2017 11:09

kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 25, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 26, 2017

arthurprs force-pushed the iter-position-bounds-check branch from 3529709 to d2dc4a4Compare October 26, 2017 21:00

bluss reviewed Oct 26, 2017
View reviewed changes

arthurprs force-pushed the iter-position-bounds-check branch from d2dc4a4 to f014dc1Compare October 27, 2017 07:48

kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2017

arthurprs force-pushed the iter-position-bounds-check branch from f014dc1 to 9591262Compare October 27, 2017 09:28

arthurprs closed this Oct 29, 2017

arthurprs deleted the iter-position-bounds-check branch October 29, 2017 17:17

bluss reviewed Oct 29, 2017
View reviewed changes

arthurprs restored the iter-position-bounds-check branch October 29, 2017 18:24

arthurprs reopened this Oct 29, 2017

arthurprs force-pushed the iter-position-bounds-check branch 2 times, most recently from ecb4f4a to 896e6a5Compare October 29, 2017 18:58

Hint slice.{r}position is within slice bounds
c8d3814

arthurprs force-pushed the iter-position-bounds-check branch from 896e6a5 to c8d3814Compare October 29, 2017 21:38

run more tests in travis
204d640

dtolnay mentioned this pull request Nov 13, 2017
Bounds check not optimized out after {r}position on a slice #45964
Closed

dtolnay closed this Nov 13, 2017

arthurprs mentioned this pull request Nov 28, 2017
[WIP] New implementation of the slice iterators using NonZero #46223
Closed

arthurprs mentioned this pull request Jan 10, 2018
Optimize slice.{r}position result bounds check #47333
Merged

arthurprs mentioned this pull request Jan 26, 2018
Use the slice length to hint the optimizer about iter.position result #47772
Merged

kennytm mentioned this pull request Feb 14, 2018
Try to fix 48116 and 48192 #48209
Merged

Uh oh!

Uh oh!

Conversation

arthurprs commented Oct 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Oct 24, 2017

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnay commented Oct 25, 2017

Uh oh!

bors commented Oct 25, 2017

Uh oh!

dtolnay commented Oct 25, 2017

Uh oh!

arthurprs commented Oct 25, 2017

Uh oh!

kennytm commented Oct 25, 2017

Uh oh!

bors commented Oct 25, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

kennytm commented Oct 26, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

kennytm commented Oct 26, 2017

Uh oh!

arthurprs commented Oct 26, 2017

Uh oh!

blussOct 26, 2017

Choose a reason for hiding this comment

Uh oh!

arthurprsOct 27, 2017

Choose a reason for hiding this comment

Uh oh!

arthurprs commented Oct 29, 2017

Uh oh!

blussOct 29, 2017

Choose a reason for hiding this comment

Uh oh!

arthurprsOct 29, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennytmOct 29, 2017

Choose a reason for hiding this comment

Uh oh!

arthurprsOct 29, 2017

Choose a reason for hiding this comment

Uh oh!

bluss commented Oct 29, 2017

Uh oh!

scottmcm commented Oct 29, 2017

Uh oh!

arthurprs commented Oct 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennytm commented Oct 30, 2017

Uh oh!

arthurprs commented Oct 30, 2017

Uh oh!

arthurprs commented Nov 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennytm commented Nov 1, 2017

Uh oh!

arthurprs commented Nov 1, 2017

Uh oh!

dtolnay commented Nov 13, 2017

Uh oh!

Reviewers

Assignees

arthurprs commented Oct 24, 2017•
edited
Loading

arthurprsOct 29, 2017•
edited
Loading

arthurprs commented Oct 30, 2017•
edited
Loading

arthurprs commented Nov 1, 2017•
edited
Loading