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

Dead code/instructions generated to detect divide by zero #44500

Closed
vitalyd opened this issue Sep 11, 2017 · 3 comments
Closed

Dead code/instructions generated to detect divide by zero #44500

vitalyd opened this issue Sep 11, 2017 · 3 comments
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@vitalyd
Copy link

vitalyd commented Sep 11, 2017

This is born out of https://users.rust-lang.org/t/division-by-nonzero/12822.

The gist is the following function:

pub fn d1(x: u64, y: u64) -> u64 { 
  x / y.saturating_add(1) 
}

generates the following x86_64 assembly:

example::d1:
  addq $1, %rsi
  movq $-1, %rcx
  cmovaeq %rsi, %rcx
  testq %rcx, %rcx
  je .LBB0_2
  xorl %edx, %edx
  movq %rdi, %rax
  divq %rcx
  retq
.LBB0_2:
  pushq %rbp
  movq %rsp, %rbp
  leaq panic_loc.2(%rip), %rdi
  callq core::panicking::panic@PLT

str.0:
  .ascii "/tmp/compiler-explorer-compiler117811-5-86yrz4.tvuxewb3xr/example.rs"

str.1:
  .ascii "attempt to divide by zero"

https://godbolt.org/g/8eh61N

As far as I can tell, that division cannot panic. If that's true, then the following inefficiences are present:

  1. The panic basic block that's generated.
  2. The testq and je instructions are essentially dead.

I don't know the innards of LLVM, but perhaps the non-zero/non-wrapping aspect isn't conveyed to LLVM here? Or perhaps it's not able to figure it out on its own based on whatever IR is given to it.

@Thiez
Copy link
Contributor

Thiez commented Sep 11, 2017

It seems LLVM fails to optimize this in C++ as well, see godbolt. GCC appears to be smarter.

@sfackler sfackler added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Sep 11, 2017
@TimNN TimNN added I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Sep 17, 2017
@nikic
Copy link
Contributor

nikic commented Dec 15, 2018

I've submitted https://reviews.llvm.org/D55735 to fix this, conditional on us switching to use saturating add intrinsics (#55286).

@nikic nikic self-assigned this Dec 15, 2018
@scottmcm
Copy link
Member

bors added a commit that referenced this issue Jan 31, 2019
Use LLVM intrinsics for saturating add/sub

Use the `[su](add|sub).sat` LLVM intrinsics, if we're compiling against LLVM 8, as they should optimize and codegen better than IR based on `[su](add|sub).with.overlow`.

For the fallback for LLVM < 8 I'm using the same expansion that target lowering in LLVM uses, which is not the same as Rust currently uses (in particular due to the use of selects rather than branches).

Fixes #55286.
Fixes #52203.
Fixes #44500.

r? @nagisa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. 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

6 participants