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

rustc generates badly optimized for wrapping_div #34634

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
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-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