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

Suboptimal debug codegen for integer division with a constant rhs #72751

Open
alexcrichton opened this issue May 29, 2020 · 9 comments · Fixed by #106340
Open

Suboptimal debug codegen for integer division with a constant rhs #72751

alexcrichton opened this issue May 29, 2020 · 9 comments · Fixed by #106340
Labels
A-codegen Area: Code generation A-mir-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

alexcrichton commented May 29, 2020

Codegen for division in general needs to generate code to panic if the division will panic (e.g. divide by zero or integer overflow). In the case of constants, however, it looks like rustc almost optimizes debug codegen but not quite. For example this code:

#[no_mangle]
pub extern fn foo(a: i32) -> i32 {
     a / 256
}

generates this IR in debug mode:

define i32 @foo(i32 %a) unnamed_addr #0 !dbg !6 {
start:
  %a.dbg.spill = alloca i32, align 4
  store i32 %a, i32* %a.dbg.spill, align 4
  call void @llvm.dbg.declare(metadata i32* %a.dbg.spill, metadata !12, metadata !DIExpression()), !dbg !13
  %_4 = icmp eq i32 %a, -2147483648, !dbg !14
  %_5 = and i1 false, %_4, !dbg !14
  %0 = call i1 @llvm.expect.i1(i1 %_5, i1 false), !dbg !14
  br i1 %0, label %panic, label %bb1, !dbg !14

bb1:                                              ; preds = %start
  %1 = sdiv i32 %a, 256, !dbg !14
  ret i32 %1, !dbg !15

panic:                                            ; preds = %start
; call core::panicking::panic
  call void @_ZN4core9panicking5panic17h8a9eda1e10298363E([0 x i8]* noalias nonnull readonly align 1 bitcast ([31 x i8]* @str.0 to [0 x i8]*), i64 31, %"core::panic::Location"* noalias readonly align 8 dereferenceable(24) bitcast (<{ i8*, [16 x i8] }>* @anon.670bbfb7b436af44f45c978390c94a0e.1 to %"core::panic::Location"*)), !dbg !14
  unreachable, !dbg !14
}

It looks like rustc is "smart enough" to generate %_5 = and i1 false, %_4, !dbg !14 to realize the rhs is not negative one and it's not zero, but it's not quite smart enough to know that false && x is always false, so it still generates a branch to panic.

@alexcrichton alexcrichton added the A-codegen Area: Code generation label May 29, 2020
@jonas-schievink jonas-schievink added A-mir-opt Area: MIR optimizations 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 May 29, 2020
alexcrichton added a commit to alexcrichton/libm that referenced this issue May 29, 2020
This commit moves over more array accesses to the `i!` macro to avoid
bounds checks when debug assertions are disabled. This is surfaced from
rust-lang/compiler-builtins#360 where recent changes in codegen units
has caused some bounds checks to not get elided in release mode. This
also adds a `div!` macro to work around rust-lang/rust#72751.
alexcrichton added a commit to alexcrichton/libm that referenced this issue May 29, 2020
This commit moves over more array accesses to the `i!` macro to avoid
bounds checks when debug assertions are disabled. This is surfaced from
rust-lang/compiler-builtins#360 where recent changes in codegen units
has caused some bounds checks to not get elided in release mode. This
also adds a `div!` macro to work around rust-lang/rust#72751.
alexcrichton added a commit to alexcrichton/libm that referenced this issue May 29, 2020
This commit moves over more array accesses to the `i!` macro to avoid
bounds checks when debug assertions are disabled. This is surfaced from
rust-lang/compiler-builtins#360 where recent changes in codegen units
has caused some bounds checks to not get elided in release mode. This
also adds a `div!` macro to work around rust-lang/rust#72751.
alexcrichton added a commit to rust-lang/libm that referenced this issue May 29, 2020
* Use macros for more division/array checks

This commit moves over more array accesses to the `i!` macro to avoid
bounds checks when debug assertions are disabled. This is surfaced from
rust-lang/compiler-builtins#360 where recent changes in codegen units
has caused some bounds checks to not get elided in release mode. This
also adds a `div!` macro to work around rust-lang/rust#72751.

* Don't test/bench our shim crate

It's not intended to run all our tests
@alex
Copy link
Member

alex commented May 30, 2020

I assume the task here is teaching the MIR optimizer to constant fold false && x to false? If someone can suggest where in the codebase such an optimization might live, I'd be interested to take a look.

@xldenis
Copy link
Contributor

xldenis commented Jul 22, 2020

fixed by #74491

@camelid
Copy link
Member

camelid commented Apr 12, 2021

fixed by #74491

Should this be closed then?

@alex
Copy link
Member

alex commented Apr 12, 2021

This doesn't appear to be fixed by default: https://rust.godbolt.org/z/K8zx5nrYW

@xldenis
Copy link
Contributor

xldenis commented Apr 12, 2021

You need to turn on optimizations with -O then it gets reduced to a single sdiv.

https://rust.godbolt.org/z/hfPY1T7x4


define i32 @foo(i32 %a) unnamed_addr #0 !dbg !6 {
  %0 = sdiv i32 %a, 256, !dbg !10
  ret i32 %0, !dbg !11
}

@xldenis
Copy link
Contributor

xldenis commented Apr 12, 2021

My bad, I forgot this was about debug builds, it seems like we aren't performing enough mir optimizations in the debug context?

@camelid
Copy link
Member

camelid commented Apr 13, 2021

FWIW,

pub fn foo(x: bool) -> bool {
    false && x
}

generates this MIR in debug mode:

// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.
fn foo(_1: bool) -> bool {
    debug x => _1;                       // in scope 0 at src/lib.rs:1:12: 1:13
    let mut _0: bool;                    // return place in scope 0 at src/lib.rs:1:24: 1:28

    bb0: {
        _0 = const false;                // scope 0 at src/lib.rs:2:5: 2:15
        return;                          // scope 0 at src/lib.rs:3:2: 3:2
    }
}

So it seems that in simple cases the compiler is smart enough.

@saethlin
Copy link
Member

saethlin commented Jan 1, 2023

I'm looking into this
@rustbot claim

@bors bors closed this as completed in 89e0576 Jan 9, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue Jan 13, 2023
Always permit ConstProp to exploit arithmetic identities

Fixes rust-lang/rust#72751

Initially, I thought I would need to enable operand propagation then do something else, but actually rust-lang/rust#74491 already has the fix for the issue in question! It looks like this optimization was put under MIR opt level 3 due to possible soundness/stability implications, then demoted further to MIR opt level 4 when MIR opt level 2 became associated with `--release`.

Perhaps in the past we were doing CTFE on optimized MIR? We aren't anymore, so this optimization has no stability implications.

r? `@oli-obk`
@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2023

We are causing this issue to occur again: #109900 (comment)

I was unable to find a motivation for fixing this issue beyond "nice to have if easily done". We ususally don't put a lot of effort into avoiding codegen of dead code (like the panic path here). Thus I am not reopening this issue. If you have a motivating use case, please don't hesistate to inform us of it and either open a new issue or comment here to ask for this one to get reopened.

@saethlin saethlin reopened this Apr 2, 2024
@saethlin saethlin removed their assignment Apr 4, 2024
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-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. 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.

7 participants