Skip to content

Inline u8::is_utf8_char_boundary #95407

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged

Conversation

KamilaBorowska
Copy link
Contributor

@KamilaBorowska KamilaBorowska commented Mar 28, 2022

Since Rust beta, Rust is incapable of inlining this function in the following example function.

pub fn safe_substr_to(s: &str, mut length: usize) -> &str {
    loop {
        if let Some(s) = s.get(..length) {
            return s;
        }
        length -= 1;
    }
}

When compiled with beta or nightly compiler on Godbolt with -C opt-level=3 flag it prints the following assembly.

example::safe_substr_to:
        push    r15
        push    r14
        push    r12
        push    rbx
        push    rax
        mov     r14, rdi
        test    rdx, rdx
        je      .LBB0_8
        mov     rbx, rdx
        mov     r15, rsi
        mov     r12, qword ptr [rip + core::num::<impl u8>::is_utf8_char_boundary@GOTPCREL]
        jmp     .LBB0_4
.LBB0_2:
        je      .LBB0_9
.LBB0_3:
        add     rbx, -1
        je      .LBB0_8
.LBB0_4:
        cmp     rbx, r15
        jae     .LBB0_2
        movzx   edi, byte ptr [r14 + rbx]
        call    r12
        test    al, al
        je      .LBB0_3
        mov     r15, rbx
        jmp     .LBB0_9
.LBB0_8:
        xor     r15d, r15d
.LBB0_9:
        mov     rax, r14
        mov     rdx, r15
        add     rsp, 8
        pop     rbx
        pop     r12
        pop     r14
        pop     r15
        ret

qword ptr [rip + core::num::<impl u8>::is_utf8_char_boundary@GOTPCREL] is not inlined. -C remark=all outputs the following message:

note: /rustc/7bccde19767082c7865a12902fa614ed4f8fed73/library/core/src/str/mod.rs:214:25: inline: _ZN4core3num20_$LT$impl$u20$u8$GT$21is_utf8_char_boundary17hace9f12f5ba07a7fE will not be inlined into _ZN4core3str21_$LT$impl$u20$str$GT$16is_char_boundary17hf2587e9a6b8c5e43E because its definition is unavailable

Stable compiler outputs more reasonable code:

example::safe_substr_to:
        mov     rcx, rdx
        mov     rax, rdi
        test    rdx, rdx
        je      .LBB0_9
        mov     rdx, rsi
        jmp     .LBB0_4
.LBB0_2:
        cmp     rdx, rcx
        je      .LBB0_7
.LBB0_3:
        add     rcx, -1
        je      .LBB0_9
.LBB0_4:
        cmp     rcx, rdx
        jae     .LBB0_2
        cmp     byte ptr [rax + rcx], -64
        jl      .LBB0_3
        mov     rdx, rcx
.LBB0_7:
        ret
.LBB0_9:
        xor     edx, edx
        ret

@rust-highfive
Copy link
Contributor

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2022
@clarfonthey
Copy link
Contributor

Oof, sorry for missing that when I originally factored that function out. Thank you for fixing it!

@scottmcm
Copy link
Member

Given that it's pub(crate) I'm surprised it's needed. I guess something's getting unlucky with CGUs?

But it's a trivial and non-generic method, so adding #[inline] is certainly reasonable.

r? @scottmcm
@bors r+ rollup

(If the refactor didn't change rustc perf enough to notice this, I don't think fixing it will affect rustc perf either.)

@bors
Copy link
Collaborator

bors commented Mar 28, 2022

📌 Commit 12c085a has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93787 (parallel_compiler: hide dependencies behind feature)
 - rust-lang#95318 (diagnostics: correct generic bounds with doubled colon)
 - rust-lang#95328 (Fix yet another Box<T, A> ICE)
 - rust-lang#95397 (Link to std::io's platform-specific behavior disclaimer)
 - rust-lang#95407 (Inline u8::is_utf8_char_boundary)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1f33cd1 into rust-lang:master Mar 28, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 28, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants