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.

Sign up for GitHub

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

Jump to bottom

Bounds check not optimized out after {r}position on a slice #45964

Closed
dtolnay opened this issue Nov 13, 2017 · 6 comments · Fixed by #78252
Closed

Bounds check not optimized out after {r}position on a slice #45964

dtolnay opened this issue Nov 13, 2017 · 6 comments · Fixed by #78252
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Nov 13, 2017

All the unrolling makes it hard for llvm to optimize it out. Sample: https://godbolt.org/g/R9nLvC

#45501 has a fix that involves assume(index < len) but we couldn't quite get it to pass tests. Will need to give it another try with some fresh eyes.
@dtolnay dtolnay mentioned this issue Nov 13, 2017
Closed
@kennytm
Copy link
Member

kennytm commented Nov 13, 2017

According to #44899 (comment) there was a fix in upstream LLVM. We may reopen the PR if we backport the fix.

@arielb1
Copy link
Contributor

arielb1 commented Nov 13, 2017

@kennytm

I already backported the fix, it's just that someone needs to make an "up the LLVM" PR that includes it.

@TimNN TimNN added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Nov 14, 2017
@arthurprs arthurprs mentioned this issue Jan 10, 2018
Merged
kennytm added a commit to kennytm/rust that referenced this issue Jan 17, 2018
@kennytm
Rollup merge of rust-lang#47333 - arthurprs:iter-position-bounds-chec…
175dd84 
…k, r=dtolnay

Optimize slice.{r}position result bounds check

Second attempt of rust-lang#45501
Fixes rust-lang#45964

Demo: https://godbolt.org/g/N4mBHp
@arthurprs arthurprs mentioned this issue Jan 26, 2018
Merged
bors added a commit that referenced this issue Jan 28, 2018
@bors
Auto merge of #47772 - arthurprs:iter-position-bounds-check, r=dtolnay
0119b44 
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 #45501
Fixes #45964
@kennytm
Copy link
Member

kennytm commented Feb 14, 2018 •
edited
Loading

Reopening this because of #48209.

(We can close again if it turns out the assume is not necessary for optimizing out the bounds check on LLVM 6.)

@kennytm kennytm reopened this Feb 14, 2018
@XAMPPRocky XAMPPRocky added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 19, 2018
@scottmcm
Copy link
Member

scottmcm commented Apr 1, 2018

It's possible #49551 could help with this, as it should make it easier for LLVM to understand the length of a slice iterator 🤞

@arthurprs
Copy link
Contributor

I think this can be closed, they put the assume back after patching LLVM.

@arielb1
Copy link
Contributor

arielb1 commented Apr 1, 2018

Could we have a codegen test? I'm marking this as E-needstest.

@arielb1 arielb1 added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 1, 2018
@bugadani bugadani mentioned this issue Oct 22, 2020
Merged
bugadani added a commit to bugadani/rust that referenced this issue Oct 22, 2020
@bugadani
Add codegen test for rust-lang#45964
3815c6a
@Dylan-DPC-zz Dylan-DPC-zz mentioned this issue Oct 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 26, 2020
@bors
Auto merge of rust-lang#78387 - Dylan-DPC:rollup-ch0st6z, r=Dylan-DPC
1cd97ca 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#74477 (`#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm)
 - rust-lang#77836 (transmute_copy: explain that alignment is handled correctly)
 - rust-lang#78126 (Properly define va_arg and va_list for aarch64-apple-darwin)
 - rust-lang#78137 (Initialize tracing subscriber in compiletest tool)
 - rust-lang#78161 (Add issue template link to IRLO)
 - rust-lang#78214 (Tweak match arm semicolon removal suggestion to account for futures)
 - rust-lang#78247 (Fix rust-lang#78192)
 - rust-lang#78252 (Add codegen test for rust-lang#45964)
 - rust-lang#78268 (Do not try to report on closures to avoid ICE)
 - rust-lang#78295 (Add some regression tests)

Failed merges:

r? `@ghost`
@bors bors closed this as completed in 752bce5 Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add codegen test for #45964 bugadani/rust
7 participants
@kennytm @arthurprs @TimNN @arielb1 @dtolnay @XAMPPRocky @scottmcm