Skip to content

Missed optimization: computing smaller index than length does not elliminate bound check #80963

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
newpavlov opened this issue Jan 13, 2021 · 5 comments
Assignees
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. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@newpavlov
Copy link
Contributor

newpavlov commented Jan 13, 2021

Code:

pub fn foo(buf: &[u8]) -> Option<&[u8]> {
    // guard against potential overflow
    if buf.len() > usize::MAX/3 {
        return None;
    }
    let n = (3*buf.len())/4;
    Some(&buf[..n])
}

godbolt

Even replacing usize::MAX/3 with any non-zero small number (even 1) does not result in removal of the bound check.

This behavior can be reproduced on current Nightly and previous stable compiler versions.

@jonas-schievink jonas-schievink added 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. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Jan 13, 2021
@the8472
Copy link
Member

the8472 commented Jan 13, 2021

Workaround:

pub fn foo(buf: &[u8]) -> Option<&[u8]> {
    if buf.len() > usize::MAX/3 {
        return None;
    }
    let n = (3*buf.len())/4;
    let n = std::cmp::min(n, buf.len());
    Some(&buf[..n])
}

@nikic
Copy link
Contributor

nikic commented Jan 13, 2021

https://bugs.llvm.org/show_bug.cgi?id=48744

@nikic nikic self-assigned this Jan 18, 2021
@nikic
Copy link
Contributor

nikic commented Jan 18, 2021

Upstream fix: llvm/llvm-project@a13c0f6

@MSxDOS
Copy link

MSxDOS commented Mar 5, 2021

Fixed on the latest nightly by #81451

@newpavlov
Copy link
Contributor Author

I confirm the fix.

# for free to join this conversation on GitHub. Already have an account? # to comment
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. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

No branches or pull requests

5 participants