Skip to content

Vec::pop unwrap panic branch not optimized out for types with niche opts #107310

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
y21 opened this issue Jan 25, 2023 · 2 comments
Closed

Vec::pop unwrap panic branch not optimized out for types with niche opts #107310

y21 opened this issue Jan 25, 2023 · 2 comments
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

@y21
Copy link
Member

y21 commented Jan 25, 2023

This code:

use std::ptr::NonNull;

// type Value = NonNull<i64>;
type Value = *const i64;

pub fn pop2(stack: &mut Vec<Value>) -> (Value, Value) {
    assert!(stack.len() >= 2);
    let b = stack.pop().unwrap();
    let a = stack.pop().unwrap();
    (a, b)
}

optimizes differently based on if Value is *const i64 or NonNull<i64> (or apparently any other type with niche optimizations, like &i64 or NonZeroI64).

Reproducer

Specifically, *const i64 has the unwrap panic checks optimized out,

        push    rax
        mov     rax, qword ptr [rdi + 16]
        cmp     rax, 1
        jbe     .LBB0_1 ; assert
        mov     rcx, qword ptr [rdi + 8]
        mov     rdx, qword ptr [rcx + 8*rax - 8]
        lea     rsi, [rax - 2]
        mov     qword ptr [rdi + 16], rsi
        mov     rax, qword ptr [rcx + 8*rax - 16]
        pop     rcx
        ret

while NonNull<i64> keeps them.

example::pop2:
        cmp     rax, 2
        jb      .LBB0_7 ; assert
        ...
        test    rdx, rdx
        je      .LBB0_2 ; unwrap 1
        ...
        test    rax, rax
        je      .LBB0_5 ; unwrap 2

Meta

rustc --version --verbose:

rustc 1.69.0-nightly (c8e6a9e8b 2023-01-23)
binary: rustc
commit-hash: c8e6a9e8b6251bbc8276cb78cabe1998deecbed7
commit-date: 2023-01-23
host: x86_64-unknown-linux-gnu
release: 1.69.0-nightly
LLVM version: 15.0.7

@y21 y21 added the C-bug Category: This is a bug. label Jan 25, 2023
@Noratrieb
Copy link
Member

Vec::pop returns Option<T>, so NonNull gets a niche opt there, which confuses LLVM.
-Copt-level=3 --emit llvm-ir -Cno-prepopulate-passes shows what I assume is the key difference:

- %_9 = load i64, ptr %self, align 8, !dbg !129, !range !119, !noundef !12
+ %_9 = select i1 %4, i64 0, i64 1, !dbg !147

For Option<*const i64> we just load the discriminant from the option.
For Option<NonNull<i64>> we need a select to compute the discriminant.

@Noratrieb Noratrieb 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 25, 2023
@y21
Copy link
Member Author

y21 commented Jan 25, 2023

Actually this looks like a duplicate of #71257. Closing.

@y21 y21 closed this as completed Jan 25, 2023
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