Skip to content

rustc generates badly optimized for wrapping_div #34634

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

Closed
eefriedman opened this issue Jul 3, 2016 · 5 comments · Fixed by #76961
Closed

rustc generates badly optimized for wrapping_div #34634

eefriedman opened this issue Jul 3, 2016 · 5 comments · Fixed by #76961
Labels
A-codegen Area: Code generation A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@eefriedman
Copy link
Contributor

pub fn f(x: i32, y: i32) -> i32 {
  x.wrapping_div(y)
}

Gives:

_ZN8rust_out1f17h164f16efff59de66E:
    pushq   %rax
.Ltmp0:
    cmpl    $-2147483648, %edi
    jne .LBB0_2
    movl    $-2147483648, %eax
    cmpl    $-1, %esi
    je  .LBB0_5
.LBB0_2:
    cmpl    $-1, %esi
    je  .LBB0_6
    testl   %esi, %esi
    jne .LBB0_4
    leaq    panic_loc7436(%rip), %rdi
    callq   _ZN4core9panicking5panic17h907815f47e914305E@PLT
.LBB0_6:
    cmpl    $-2147483648, %edi
    je  .LBB0_7
.LBB0_4:
    movl    %edi, %eax
    cltd
    idivl   %esi
.LBB0_5:
    popq    %rcx
    retq
.LBB0_7:
    leaq    panic_loc7440(%rip), %rdi
    callq   _ZN4core9panicking5panic17h907815f47e914305E@PLT

Specifically, .LBB0_7 is unreachable, but LLVM can't quite figure that out. It's probably possible to write wrapping_div in a way which avoids this problem.

@eefriedman eefriedman changed the title rustc generates bad code for wrapping_div rustc generates badly optimized for wrapping_div Jul 3, 2016
@Aatch
Copy link
Contributor

Aatch commented Jul 4, 2016

Not sure why LLVM doesn't optimise this more (the duplicated check of %esi against -1 should be easy enough to handle), but the good news is that the MIR-driven code generator produces this ASM instead:

    cmpl    $-2147483648, %edi
    jne .LBB0_2
    movl    $-2147483648, %eax
    cmpl    $-1, %esi
    je  .LBB0_3
.LBB0_2:
    movl    %edi, %eax
    cltd
    idivl   %esi
.LBB0_3:
    retq

Which is what is expected.

@brson brson added I-slow Issue: Problems and improvements with respect to performance of generated code. A-codegen Area: Code generation A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Jul 8, 2016
@nagisa
Copy link
Member

nagisa commented Apr 1, 2017

Currently generated code looks just fine to me. Fixed.

@arielb1 arielb1 added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 1, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 25, 2017
@oyvindln
Copy link
Contributor

oyvindln commented Sep 7, 2017

Still looks fine it seems: (latest stable)

	cmpl	$-2147483648, %edi
	jne	.LBB0_2
	movl	$-2147483648, %eax
	cmpl	$-1, %esi
	je	.LBB0_4
.LBB0_2:
	testl	%esi, %esi
	je	.LBB0_5
	movl	%edi, %eax
	cltd
	idivl	%esi
.LBB0_4:
	popq	%rcx
	retq
.LBB0_5:
	leaq	panic_loc.2(%rip), %rdi
	callq	_ZN4core9panicking5panic17hec1812dcc135e139E@PLT

@nox
Copy link
Contributor

nox commented Apr 2, 2018

Given this just needs a test, this could be transformed into an easy issue for newcomers.

@steveklabnik
Copy link
Member

Triage; not aware of any test added. Still gives the good code when compiled.

bugadani added a commit to bugadani/rust that referenced this issue Sep 20, 2020
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 21, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 21, 2020
Rollup of 13 pull requests

Successful merges:

 - rust-lang#76135 (Stabilize some Option methods as const)
 - rust-lang#76628 (Add sample defaults for config.toml )
 - rust-lang#76846 (Avoiding unnecesary allocations at rustc_errors)
 - rust-lang#76867 (Use intra-doc links in core/src/iter when possible)
 - rust-lang#76868 (Finish moving to intra doc links for std::sync)
 - rust-lang#76872 (Remove DeclareMethods)
 - rust-lang#76936 (Add non-`unsafe` `.get_mut()` for `Unsafecell`)
 - rust-lang#76958 (Replace manual as_nanos and as_secs_f64 reimplementations)
 - rust-lang#76959 (Replace write_fmt with write!)
 - rust-lang#76961 (Add test for issue rust-lang#34634)
 - rust-lang#76962 (Use const_cstr macro in consts.rs)
 - rust-lang#76963 (Remove unused static_assert macro)
 - rust-lang#77000 (update Miri)

Failed merges:

r? `@ghost`
@bors bors closed this as completed in 5760b08 Sep 21, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-codegen Area: Code generation A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants