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

Matching on ASCII digits no longer optimized #123305

Open
krtab opened this issue Mar 31, 2024 · 2 comments
Open

Matching on ASCII digits no longer optimized #123305

krtab opened this issue Mar 31, 2024 · 2 comments
Assignees
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-slow Issue: Problems and improvements with respect to performance of generated code. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@krtab
Copy link
Contributor

krtab commented Mar 31, 2024

Code

This

pub fn f(character: u8) -> Option<u8> {
    match character {
        b'0' => Some(0),
        b'1' => Some(1),
        b'2' => Some(2),
        b'3' => Some(3),
        b'4' => Some(4),
        b'5' => Some(5),
        b'6' => Some(6),
        b'7' => Some(7),
        b'8' => Some(8),
        b'9' => Some(9),
        _ => None,
    }
}

used in 1.76.0 to be optimized to

f:
        lea     edx, [rdi - 48]
        cmp     dl, 10
        setb    al
        ret

but in 1.77 and nightly, this remains

f:
        movzx   eax, dil
        add     eax, -48
        cmp     eax, 9
        ja      .LBB0_3
        lea     rcx, [rip + .LJTI0_0]
        movsxd  rax, dword ptr [rcx + 4*rax]
        add     rax, rcx
        jmp     rax
.LBB0_2:
        mov     al, 1
        xor     edx, edx
        ret
.LBB0_3:
        xor     eax, eax
        ret
.LBB0_5:
        mov     dl, 1
        mov     al, 1
        ret
.LBB0_6:
        mov     al, 1
        mov     dl, 2
        ret
.LBB0_7:
        mov     al, 1
        mov     dl, 3
        ret
.LBB0_8:
        mov     al, 1
        mov     dl, 4
        ret
.LBB0_9:
        mov     al, 1
        mov     dl, 5
        ret
.LBB0_10:
        mov     al, 1
        mov     dl, 6
        ret
.LBB0_11:
        mov     al, 1
        mov     dl, 7
        ret
.LBB0_12:
        mov     al, 1
        mov     dl, 9
        ret
.LBB0_13:
        mov     al, 1
        mov     dl, 8
        ret
.LJTI0_0:
        .long   .LBB0_2-.LJTI0_0
        .long   .LBB0_5-.LJTI0_0
        .long   .LBB0_6-.LJTI0_0
        .long   .LBB0_7-.LJTI0_0
        .long   .LBB0_8-.LJTI0_0
        .long   .LBB0_9-.LJTI0_0
        .long   .LBB0_10-.LJTI0_0
        .long   .LBB0_11-.LJTI0_0
        .long   .LBB0_13-.LJTI0_0
        .long   .LBB0_12-.LJTI0_0

Interestingly,

pub fn f(character: u8) -> Option<u32> {
    match character {
        b'0' => Some(0),
        b'1' => Some(1),
        b'2' => Some(2),
        b'3' => Some(3),
        b'4' => Some(4),
        b'5' => Some(5),
        b'6' => Some(6),
        b'7' => Some(7),
        b'8' => Some(8),
        b'9' => Some(9),
        _ => None,
    }
}

and

pub fn f(character: u8) -> Option<u8>
{
    match character {
        b'0' => Some(0),
        b'1' => Some(1),
        b'2' => Some(2),
        b'3' => Some(3),
        b'4' => Some(4),
        b'5' => Some(5),
        b'6' => Some(6),
        b'7' => Some(7),
        //b'8' => Some(8),
        //b'9' => Some(9),
        _ => None,
    }
}

Get well optimized.

Bisected to #118991

@rustbot modify labels: +regression-from-stable-to-stable

I'd like to have a go at this so @rustbot claim

@krtab krtab added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Mar 31, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Mar 31, 2024
@krtab
Copy link
Contributor Author

krtab commented Mar 31, 2024

@rustbot label -regression-untriaged

@rustbot rustbot removed the regression-untriaged Untriaged performance or correctness regression. label Mar 31, 2024
@saethlin saethlin added the I-slow Issue: Problems and improvements with respect to performance of generated code. label Apr 1, 2024
@apiraino
Copy link
Contributor

apiraino commented Apr 2, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 2, 2024
@jieyouxu jieyouxu added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-slow Issue: Problems and improvements with respect to performance of generated code. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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