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

Missed optimization with core::cmp::{min,max} {<,>} n and the range of one the values is known #92967

Closed
paolobarbolini opened this issue Jan 16, 2022 · 3 comments
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@paolobarbolini
Copy link
Contributor

paolobarbolini commented Jan 16, 2022

I tried this code:

#![feature(core_intrinsics)]

pub unsafe fn foo1(a: usize, b: usize) {
    std::intrinsics::assume(a < 6);

    assert!(a.min(b) < 6);
}

I expected to see this happen: the assert is optimized away. By proving a < 6, even without knowing b, we can assume a.min(b) < 6.

Instead, this happened: it doesn't optimize, but it does in the following conditions:

#![feature(core_intrinsics)]

pub unsafe fn foo2(a: usize, b: usize) {
    std::intrinsics::assume(a < 6);

    assert!(a < 6 || b < 6);

    assert!((if a < b { a } else { b }) < 6);
    assert!((if a <= b { a } else { b }) < 6);
}

// or...

pub unsafe fn foo3(a: usize, b: usize) {
    std::intrinsics::assume(a == 3);

    assert!(a.min(b) < 6);
}


// or...

pub unsafe fn foo4(a: usize, b: usize) {
    std::intrinsics::assume(a < 6);
    std::intrinsics::assume(b < 6);

    assert!(a.min(b) < 6);
}

Why this matters

Prevents the ptr_rotate implementation from optimizing here

} else if cmp::min(left, right) <= mem::size_of::<BufType>() / mem::size_of::<T>() {

which would reduce the amount of assembly generated by #89714

Meta

rustc --version --verbose:

rustc 1.60.0-nightly (ec4bcaac4 2022-01-15)
binary: rustc
commit-hash: ec4bcaac450279b029f3480b8b8f1b82ab36a5eb
commit-date: 2022-01-15
host: x86_64-unknown-linux-gnu
release: 1.60.0-nightly
LLVM version: 13.0.0
@paolobarbolini paolobarbolini added the C-bug Category: This is a bug. label Jan 16, 2022
@nikic nikic 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 Jan 16, 2022
@nikic
Copy link
Contributor

nikic commented Jan 16, 2022

Godbolt: https://rust.godbolt.org/z/78sGo6na7 Still doesn't optimize on LLVM head: https://llvm.godbolt.org/z/nTPM51W7K

It looks like LVI currently only handles the case where both sides of the min/max have a known range: https://github.com/llvm/llvm-project/blob/30a4020a7db866990199f523a60ad474d7ec3efa/llvm/lib/Analysis/LazyValueInfo.cpp#L810 Should be easy to fix at least.

@nikic
Copy link
Contributor

nikic commented Jan 18, 2022

Should be fixed by llvm/llvm-project@202d590, llvm/llvm-project@d15823e, llvm/llvm-project@9e68557.

@paolobarbolini
Copy link
Contributor Author

I was able to confirm that #93577 fixed it
https://rust.godbolt.org/z/o6bqsKEjs

Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

No branches or pull requests

2 participants