Skip to content

Missed optimization: The optimizer forgets that Some(function) is Some(_) #74425

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

Closed
yvt opened this issue Jul 17, 2020 · 3 comments · Fixed by #74533
Closed

Missed optimization: The optimizer forgets that Some(function) is Some(_) #74425

yvt opened this issue Jul 17, 2020 · 3 comments · Fixed by #74533
Assignees
Labels
C-bug Category: This is a bug. 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

@yvt
Copy link
Contributor

yvt commented Jul 17, 2020

#![no_std]

extern "C" {
    fn ext_fn0();
}

pub fn test() {
    test_inner(Some(inner0));
}

#[inline(always)]
fn test_inner(f_maybe: Option<fn()>) {
    if let Some(f) = f_maybe {
        f();
    }
}

#[inline(always)]
fn inner0() {
    unsafe { ext_fn0() };
}

I expected to see this happen: The branch in test_inner is optimized out and the test function compiles down to a single call to ext_fn0.

; expected
example::test:
        b       ext_fn0

Instead, this happened: The compiled test function includes a branch that checks if Some(inner0) is Some(_) or not.

; -Copt-level=3 -Cpanic=abort --target=thumbv7em-none-eabihf -Clto=on
example::test:
        movw    r0, :lower16:example::inner0
        movt    r0, :upper16:example::inner0
        cmp     r0, #0
        it      ne
        bne     ext_fn0
        bx      lr

example::inner0:
        b       ext_fn0

Compiler Explorer (Nightly, 1.45.0)

Meta

rustc --version --verbose:

rustc 1.46.0-nightly (23744c84d 2020-07-14)
binary: rustc
commit-hash: 23744c84d9c0f8e4e870edb983f1ad6d33449c34
commit-date: 2020-07-14
host: x86_64-unknown-linux-gnu
release: 1.46.0-nightly
LLVM version: 10.0

@yvt yvt added the C-bug Category: This is a bug. label Jul 17, 2020
@jonas-schievink jonas-schievink 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. labels Jul 17, 2020
@MSxDOS
Copy link

MSxDOS commented Jul 19, 2020

The regression starts with Rust 1.38.0

@nikic
Copy link
Contributor

nikic commented Jul 19, 2020

IR contains this weirdness: (i1 icmp ule ({}* bitcast (void ()* @_ZN7example6inner017hec4f5594ccddd419E to {}*), {}* null) Because it's a constexpr, the ule null probably doesn't get folded to ne null.

I've seen this super weird pattern emitted by rustc a few times already, wonder where it comes from.

@nikic
Copy link
Contributor

nikic commented Jul 19, 2020

Most likely from here:

bx.icmp(IntPredicate::IntULE, relative_discr, relative_max)
The zero case is already special-cased, we should emit IntEQ in that case instead of IntULE.

@nikic nikic self-assigned this Jul 19, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 8, 2020
Emit == null instead of <= null for niche check

When the niche maximum is zero, emit a "== zero" check instead of a "<= zero" check. In particular, this avoids the awkward case of "<= null". While LLVM does canonicalize this to "== null", this apparently doesn't happen for constant expressions, leading to the issue in rust-lang#74425. While that can be addressed on the LLVM side, it still seems prudent to emit sensible IR here, because this will allow null checks to be optimized earlier in the pipeline.

Fixes rust-lang#74425.
@bors bors closed this as completed in 7e5c7cf Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. 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

Successfully merging a pull request may close this issue.

4 participants