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

Boolean optimization no longer works #62993

Closed
pedantic79 opened this issue Jul 25, 2019 · 0 comments · Fixed by #63431
Closed

Boolean optimization no longer works #62993

pedantic79 opened this issue Jul 25, 2019 · 0 comments · Fixed by #63431
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pedantic79
Copy link

I'm unsure if this is a bug or not. The following code used to perform just as well as if there were no let statements. However, sometime since last year, that has changed.

pub fn is_leap_year1(year: i32) -> bool {
    let div_4 = year % 4 == 0;
    let div_100 = year % 100 == 0;
    let div_400 = year % 400 == 0;
    div_4 && !(div_100 && !div_400)
}

Starting with rust 1.33, all the modulos are always computed. While in 1.32, it would stop processing if the year was not divisible by 4. Using compiler explorer, I was able to find when is_leap_year assembly output changed. (link to godbolt with assembly output).

I wanted to make sure it wasn't a change in LLVM, so I looked at the llvm-ir. The same logic between the llvm-ir and assembly existed. (link to godbolt with llvm-ir output)

Next was to determine exactly when this change occurred, nightly-2018-12-24 had the optimization and nightly-2018-12-26 did not. Unfortunately, nightly-2018-12-25 won't install via rustup. I was however able to manually download rustc and std, and was able to verify that it is broken then.

root@2c92852f7036:~# rustc --version
rustc 1.33.0-nightly (f960f377f 2018-12-24)
root@2c92852f7036:~# rustc --crate-type=lib -C opt-level=3 --emit=llvm-ir leap.rs
root@2c92852f7036:~# sed -n -e '/is_leap_year1/,/}/ p' leap.ll
; leap::is_leap_year1
; Function Attrs: norecurse nounwind nonlazybind readnone uwtable
define zeroext i1 @_ZN4leap13is_leap_year117ha0a43b53679ed341E(i32 %year) unnamed_addr #0 {
start:
  %0 = and i32 %year, 3
  %1 = icmp eq i32 %0, 0
  %2 = srem i32 %year, 100
  %3 = icmp ne i32 %2, 0
  %4 = srem i32 %year, 400
  %5 = icmp eq i32 %4, 0
  %. = or i1 %5, %3
  %_0.0 = and i1 %1, %.
  ret i1 %_0.0
}

So some commit between these two nightly removed this optimization:

  • rustc 1.33.0-nightly (ddab10a69 2018-12-23)
  • rustc 1.33.0-nightly (f960f377f 2018-12-24)
@jonas-schievink jonas-schievink added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 25, 2019
bors pushed a commit that referenced this issue Aug 10, 2019
This reverts commit e38e954.

llvm were not able to optimize the code that well with the simplified mir.

Closes: #62993
bors added a commit that referenced this issue Aug 10, 2019
Revert "Simplify MIR generation for logical ops"

This reverts commit e38e954.

llvm were not able to optimize the code that well with the simplified mir.

Closes: #62993
Centril added a commit to Centril/rust that referenced this issue Aug 12, 2019
… r=matthewjasper

Revert "Simplify MIR generation for logical ops"

This reverts commit e38e954.

llvm were not able to optimize the code that well with the simplified mir.

Closes: rust-lang#62993
Centril added a commit to Centril/rust that referenced this issue Aug 12, 2019
… r=matthewjasper

Revert "Simplify MIR generation for logical ops"

This reverts commit e38e954.

llvm were not able to optimize the code that well with the simplified mir.

Closes: rust-lang#62993
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 2, 2021
…nd_to_get_better_asm, r=nagisa

Simplify logical operations CFG

This is basically same commit as e38e954 which was reverted later in 676953f
In both cases, this changes weren't benchmarked.
e38e954 leads to missed optimization described in [this issue](rust-lang#62993)
676953f leads to missed optimization described in [this issue](rust-lang#83623)
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-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants