Skip to content

Rework reserve assumes for Vec and String - #119465

Closed
SUPERCILEX wants to merge 4 commits into
rust-lang:masterfrom
SUPERCILEX:reserve-assume
Closed

Rework reserve assumes for Vec and String#119465
SUPERCILEX wants to merge 4 commits into
rust-lang:masterfrom
SUPERCILEX:reserve-assume

Conversation

@SUPERCILEX

Copy link
Copy Markdown
Contributor

Code:

pubfnzzz(v:&mutVec<u8>,s:&[u8]){
v.reserve(s.len());
v.extend_from_slice(s);}
pubfnzzz_str(v:&mutString,s:&str){
v.reserve(s.len());
v.push_str(s);}

Before:

.section .text.tracing_tracy::zzz,"ax",@progbits
.globl tracing_tracy::zzz
.p2align 4, 0x90
.type tracing_tracy::zzz,@function
tracing_tracy::zzz:
.cfi_startproc
push r15
.cfi_def_cfa_offset 16
push r14
.cfi_def_cfa_offset 24
push r12
.cfi_def_cfa_offset 32
push rbx
.cfi_def_cfa_offset 40
push rax
.cfi_def_cfa_offset 48
.cfi_offset rbx, -40
.cfi_offset r12, -32
.cfi_offset r14, -24
.cfi_offset r15, -16
mov rbx, rdx
mov r15, rsi
mov r14, rdi
mov rax, qword ptr [rdi]
mov r12, qword ptr [rdi + 16]
sub rax, r12
cmp rax, rdx
jb .LBB20_1
cmp rax, rbx
jb .LBB20_3
.LBB20_4:
mov rdi, qword ptr [r14 + 8]
add rdi, r12
mov rsi, r15
mov rdx, rbx
call qword ptr [rip + memcpy@GOTPCREL]
add r12, rbx
mov qword ptr [r14 + 16], r12
add rsp, 8
.cfi_def_cfa_offset 40
pop rbx
.cfi_def_cfa_offset 32
pop r12
.cfi_def_cfa_offset 24
pop r14
.cfi_def_cfa_offset 16
pop r15
.cfi_def_cfa_offset 8
ret
.LBB20_1:
.cfi_def_cfa_offset 48
mov rdi, r14
mov rsi, r12
mov rdx, rbx
call alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle
mov rax, qword ptr [r14]
mov r12, qword ptr [r14 + 16]
sub rax, r12
cmp rax, rbx
jae .LBB20_4
.LBB20_3:
mov rdi, r14
mov rsi, r12
mov rdx, rbx
call alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle
mov r12, qword ptr [r14 + 16]
jmp .LBB20_4
.section .text.tracing_tracy::zzz_str,"ax",@progbits
.globl tracing_tracy::zzz_str
.p2align 4, 0x90
.type tracing_tracy::zzz_str,@function
tracing_tracy::zzz_str:
.cfi_startproc
push r15
.cfi_def_cfa_offset 16
push r14
.cfi_def_cfa_offset 24
push r12
.cfi_def_cfa_offset 32
push rbx
.cfi_def_cfa_offset 40
push rax
.cfi_def_cfa_offset 48
.cfi_offset rbx, -40
.cfi_offset r12, -32
.cfi_offset r14, -24
.cfi_offset r15, -16
mov rbx, rdx
mov r15, rsi
mov r14, rdi
mov rax, qword ptr [rdi]
mov r12, qword ptr [rdi + 16]
sub rax, r12
cmp rax, rdx
jb .LBB27_1
cmp rax, rbx
jb .LBB27_3
.LBB27_4:
mov rdi, qword ptr [r14 + 8]
add rdi, r12
mov rsi, r15
mov rdx, rbx
call qword ptr [rip + memcpy@GOTPCREL]
add r12, rbx
mov qword ptr [r14 + 16], r12
add rsp, 8
.cfi_def_cfa_offset 40
pop rbx
.cfi_def_cfa_offset 32
pop r12
.cfi_def_cfa_offset 24
pop r14
.cfi_def_cfa_offset 16
pop r15
.cfi_def_cfa_offset 8
ret
.LBB27_1:
.cfi_def_cfa_offset 48
mov rdi, r14
mov rsi, r12
mov rdx, rbx
call alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle
mov rax, qword ptr [r14]
mov r12, qword ptr [r14 + 16]
sub rax, r12
cmp rax, rbx
jae .LBB27_4
.LBB27_3:
mov rdi, r14
mov rsi, r12
mov rdx, rbx
call alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle
mov r12, qword ptr [r14 + 16]
jmp .LBB27_4

After:

.section .text.tracing_tracy::zzz_vec,"ax",@progbits
.globl tracing_tracy::zzz_vec
.p2align 4, 0x90
.type tracing_tracy::zzz_vec,@function
tracing_tracy::zzz_vec:
.cfi_startproc
push r15
.cfi_def_cfa_offset 16
push r14
.cfi_def_cfa_offset 24
push r12
.cfi_def_cfa_offset 32
push rbx
.cfi_def_cfa_offset 40
push rax
.cfi_def_cfa_offset 48
.cfi_offset rbx, -40
.cfi_offset r12, -32
.cfi_offset r14, -24
.cfi_offset r15, -16
mov rbx, rdx
mov r15, rsi
mov r14, rdi
mov rax, qword ptr [rdi]
mov r12, qword ptr [rdi + 16]
sub rax, r12
cmp rax, rdx
jb .LBB32_1
test rbx, rbx
js .LBB32_5
.LBB32_3:
mov rdi, qword ptr [r14 + 8]
add rdi, r12
mov rax, rdi
sub rax, r15
neg rax
mov rcx, rdi
sub rcx, r15
cmovbe rcx, rax
cmp rcx, rbx
jb .LBB32_6
mov rsi, r15
mov rdx, rbx
call qword ptr [rip + memcpy@GOTPCREL]
add r12, rbx
mov qword ptr [r14 + 16], r12
add rsp, 8
.cfi_def_cfa_offset 40
pop rbx
.cfi_def_cfa_offset 32
pop r12
.cfi_def_cfa_offset 24
pop r14
.cfi_def_cfa_offset 16
pop r15
.cfi_def_cfa_offset 8
ret
.LBB32_1:
.cfi_def_cfa_offset 48
mov rdi, r14
mov rsi, r12
mov rdx, rbx
call alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle
mov r12, qword ptr [r14 + 16]
test rbx, rbx
jns .LBB32_3
.LBB32_5:
lea rdi, [rip + .L__unnamed_40]
mov esi, 71
call qword ptr [rip + core::panicking::panic_nounwind@GOTPCREL]
.LBB32_6:
lea rdi, [rip + .L__unnamed_11]
mov esi, 166
call qword ptr [rip + core::panicking::panic_nounwind@GOTPCREL]
.section .text.tracing_tracy::zzz_str,"ax",@progbits
.globl tracing_tracy::zzz_str
.p2align 4, 0x90
.type tracing_tracy::zzz_str,@function
tracing_tracy::zzz_str:
.cfi_startproc
push r15
.cfi_def_cfa_offset 16
push r14
.cfi_def_cfa_offset 24
push r12
.cfi_def_cfa_offset 32
push rbx
.cfi_def_cfa_offset 40
push rax
.cfi_def_cfa_offset 48
.cfi_offset rbx, -40
.cfi_offset r12, -32
.cfi_offset r14, -24
.cfi_offset r15, -16
mov rbx, rdx
mov r15, rsi
mov r14, rdi
mov rax, qword ptr [rdi]
mov r12, qword ptr [rdi + 16]
sub rax, r12
cmp rax, rdx
jb .LBB33_1
test rbx, rbx
js .LBB33_5
.LBB33_3:
mov rdi, qword ptr [r14 + 8]
add rdi, r12
mov rax, rdi
sub rax, r15
neg rax
mov rcx, rdi
sub rcx, r15
cmovbe rcx, rax
cmp rcx, rbx
jb .LBB33_6
mov rsi, r15
mov rdx, rbx
call qword ptr [rip + memcpy@GOTPCREL]
add r12, rbx
mov qword ptr [r14 + 16], r12
add rsp, 8
.cfi_def_cfa_offset 40
pop rbx
.cfi_def_cfa_offset 32
pop r12
.cfi_def_cfa_offset 24
pop r14
.cfi_def_cfa_offset 16
pop r15
.cfi_def_cfa_offset 8
ret
.LBB33_1:
.cfi_def_cfa_offset 48
mov rdi, r14
mov rsi, r12
mov rdx, rbx
call alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle
mov r12, qword ptr [r14 + 16]
test rbx, rbx
jns .LBB33_3
.LBB33_5:
lea rdi, [rip + .L__unnamed_40]
mov esi, 71
call qword ptr [rip + core::panicking::panic_nounwind@GOTPCREL]
.LBB33_6:
lea rdi, [rip + .L__unnamed_11]
mov esi, 166
call qword ptr [rip + core::panicking::panic_nounwind@GOTPCREL]

@rustbot

Copy link
Copy Markdown
Collaborator

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 31, 2023
@SUPERCILEX

Copy link
Copy Markdown
ContributorAuthor

The optimization only works with the inlines and assumes in the top function (unless you inline(always) the intermediate functions which probably isn't desirable).

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@SUPERCILEXSUPERCILEX changed the title Add missing reserve assume for LLVMRework reserve assumes for Vec and StringDec 31, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@bors

bors commented Jan 11, 2024

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #119864) made this pull request unmergeable. Please resolve the merge conflicts.

@m-ou-se

Copy link
Copy Markdown
Member

unless you inline(always) the intermediate functions which probably isn't desirable

The functions are tiny though. Does #[inline(always)] produce any negative results?

@m-ou-sem-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 15, 2024
@SUPERCILEX

Copy link
Copy Markdown
ContributorAuthor

Maybe this is actually an LLVM bug? I just tried putting inline(always) on every single method in the call stack and it still doesn't optimize out the reserve. The only thing that works is putting an assert_unchecked inside the top-level reserve.

@JohnCSimon

Copy link
Copy Markdown

@SUPERCILEX

ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Or if you're not going to continue, please close it. Thank you!

@SUPERCILEX

Copy link
Copy Markdown
ContributorAuthor

Yeah, after all the fiddling I've done I think this is really just an LLVM bug, so I filed: #123944.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.T-libsRelevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@SUPERCILEX@rustbot@rust-log-analyzer@bors@m-ou-se@JohnCSimon