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

Help LLVM better optimize slice::Iter(Mut)::len #61885

Merged
merged 1 commit into from Jun 18, 2019

Conversation

Projects
None yet
7 participants
@scottmcm
Copy link
Member

commented Jun 16, 2019

r? @RalfJung

I've included a codegen test that fails without this change as a demonstration of usefulness.

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

commented Jun 16, 2019

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:1e68762b:start=1560657785906660410,finish=1560657875056541206,duration=89149880796
$ 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
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:04:03]    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
[00:04:04] error[E0432]: unresolved import `crate::intrinsics::unchecked_sub`
[00:04:04]   --> src/libcore/slice/mod.rs:28:44
[00:04:04]    |
[00:04:04] 28 | use crate::intrinsics::{assume, exact_div, unchecked_sub};
[00:04:04]    |                                            |
[00:04:04]    |                                            |
[00:04:04]    |                                            no `unchecked_sub` in `intrinsics`
[00:04:04]    |                                            help: a similar name exists in the module: `unchecked_shl`
[00:04:05]    Compiling backtrace v0.3.29
[00:04:08]    Compiling compiler_builtins v0.1.16
[00:04:08]    Compiling backtrace-sys v0.1.27
[00:04:08]    Compiling cmake v0.1.38
---
travis_time:end:168cd4d0:start=1560658145141400416,finish=1560658145146148839,duration=4748423
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:12d0c46e
$ 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:1f38059e
travis_time:start:1f38059e
$ 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
[0

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)

@scottmcm scottmcm force-pushed the scottmcm:slice-iter-len-opt branch from be3ca63 to af0e35e Jun 16, 2019

@rkruppe

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Nice! r=me unless you want more eyes on this first.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Yes this looks great!

Do we have tests in src/liballoc that exhibit this? Would be nice to make sure Miri runs this code in a couple of situations, just to be sure.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Oh also, should we see if this has any perf influence?

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

Well let's get a try build.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

⌛️ Trying commit af0e35e with merge 9dd3462...

bors added a commit that referenced this pull request Jun 16, 2019

Auto merge of #61885 - scottmcm:slice-iter-len-opt, r=<try>
Help LLVM better optimize slice::Iter(Mut)::len

r? @RalfJung

I've included a codegen test that fails without this change as a demonstration of usefulness.
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

☀️ Try build successful - checks-travis
Build commit: 9dd3462

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Jun 16, 2019

Success: Queued 9dd3462 with parent 6865502, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Jun 16, 2019

Finished benchmarking try commit 9dd3462, comparison URL.

@scottmcm

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Looks like just noise in the perf run, which doesn't surprise me. (Anyone using is_empty is unaffected by this, since it's overridden separately, and it's a tiny change anyway.)

Would be nice to make sure Miri runs this code in a couple of situations, just to be sure.

Hmm, looks like I get an

error[E0080]: Miri evaluation error: unimplemented intrinsic: unchecked_sub

on the playground. I guess I should make a MIRI PR before this goes in?

Edit: I should have looked, seems like you're already on it rust-lang/miri#776 ❤️

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

I can't find any explicit tests for the slice iterator length method, but running miri-test-libstd on this PR shows that the code path does get hit, e.g. through slice::to_vec.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Ah, and this macro also gets used in nth and position and try_fold and more, so I think we are good in terms of coverage.

@bors r=rkruppe,RalfJung

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

📌 Commit af0e35e has been approved by rkruppe,RalfJung

Centril added a commit to Centril/rust that referenced this pull request Jun 17, 2019

Rollup merge of rust-lang#61885 - scottmcm:slice-iter-len-opt, r=rkru…
…ppe,RalfJung

Help LLVM better optimize slice::Iter(Mut)::len

r? @RalfJung

I've included a codegen test that fails without this change as a demonstration of usefulness.

bors added a commit that referenced this pull request Jun 17, 2019

Auto merge of #61915 - Centril:rollup-oire3i8, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #61702 (test more variants of enum-int-casting)
 - #61836 (Replace some uses of NodeId with HirId)
 - #61885 (Help LLVM better optimize slice::Iter(Mut)::len)
 - #61893 (make `Weak::ptr_eq`s into methods)
 - #61908 (don't ICE on large files)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jun 17, 2019

Auto merge of #61915 - Centril:rollup-oire3i8, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #61702 (test more variants of enum-int-casting)
 - #61836 (Replace some uses of NodeId with HirId)
 - #61885 (Help LLVM better optimize slice::Iter(Mut)::len)
 - #61893 (make `Weak::ptr_eq`s into methods)
 - #61908 (don't ICE on large files)

Failed merges:

r? @ghost

@bors bors merged commit af0e35e into rust-lang:master Jun 18, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
pr Build #20190616.9 succeeded
Details

@scottmcm scottmcm deleted the scottmcm:slice-iter-len-opt branch Jun 18, 2019

@RalfJung RalfJung referenced this pull request Jun 18, 2019

Merged

update miri #61927

bors added a commit that referenced this pull request Jun 18, 2019

Auto merge of #61927 - RalfJung:miri, r=oli-obk
update miri

Tests fail since #61885, this should fix that.

r? @oli-obk
let size = size_from_ptr(start);
if size == 0 {
// This _cannot_ use `unchecked_sub` because we depend on wrapping
// to represent the length of long ZST slice iterators.
let diff = ($self.end as usize).wrapping_sub(start as usize);
diff

This comment has been minimized.

Copy link
@vitiral

vitiral Jun 27, 2019

Contributor

There is now no need for diff here

This comment has been minimized.

Copy link
@scottmcm

scottmcm Jun 27, 2019

Author Member

@vitiral I'm quite confident it's needed, but do feel free to make a PR demonstrating otherwise.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jun 27, 2019

Member

I think they mean that the let here is redundant, and I agree. let diff = e; diff is the same as e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.