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 NonZero optimization opportunity #60044

Open
mtak- opened this issue Apr 17, 2019 · 4 comments
Open

Missed NonZero optimization opportunity #60044

mtak- opened this issue Apr 17, 2019 · 4 comments
Labels
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mtak-
Copy link
Contributor

mtak- commented Apr 17, 2019

Godbolt link

When a NonZeroUsize is created through new_unchecked and wrapped in a Some, the optimizer does not recognize that the value is always Some. This can be fixed by adding a std::intrinsics::assume(value != 0) line before creating the NonZero type.

use std::num::NonZeroUsize;
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering::Relaxed;

pub static X: AtomicUsize = AtomicUsize::new(1);

pub unsafe fn get() -> Option<NonZeroUsize> {
    let x = X.load(Relaxed);
    Some(NonZeroUsize::new_unchecked(x))
}

pub unsafe fn get2() -> usize {
    match get() {
        Some(x) => x.get(),
        None => unreachable!(), // not optimized out
    }
}

Lots of panicking code gets generated in this example.

Fixed by:

#![feature(core_intrinsics)]

use std::num::NonZeroUsize;
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering::Relaxed;

pub static X: AtomicUsize = AtomicUsize::new(1);

pub unsafe fn get() -> Option<NonZeroUsize> {
    let x = X.load(Relaxed);
    std::intrinsics::assume(x != 0); // gets rid of the panicking code
    Some(NonZeroUsize::new_unchecked(x))
}

pub unsafe fn get2() -> usize {
    match get() {
        Some(x) => x.get(),
        None => unreachable!(), // optimized away
    }
}

I would guess that adding assume inside of new_unchecked would be ok, since it's already UB if it's not zero.

Related: #51346. Posted as a new issue, because it can be fixed without any LLVM pass changes.

@jonas-schievink jonas-schievink 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 17, 2019
@scottmcm
Copy link
Member

Related: #49572

Also, assume can have detrimental effects on downstream codegen, so isn't always a good idea.

@mtak-
Copy link
Contributor Author

mtak- commented Apr 18, 2019

Ah missed #49572. Maybe this is mostly a dupe of existing issues.

@steveklabnik
Copy link
Member

Triage: no changes

@bugadani
Copy link
Contributor

I'd like to point out, that this is not caused by NonZero*, but Atomic*:
https://rust.godbolt.org/z/czeWsc

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. 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
Development

No branches or pull requests

5 participants