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 optimize if const { expr } even in opt-level=0 #85836

Closed
scottmcm opened this issue May 30, 2021 · 11 comments · Fixed by #123272
Closed

dead-code optimize if const { expr } even in opt-level=0 #85836

scottmcm opened this issue May 30, 2021 · 11 comments · Fixed by #123272
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. F-inline_const Inline constants (aka: const blocks, const expressions, anonymous constants)

Comments

@scottmcm
Copy link
Member

inline_const tracking issue: #76001

Inspired by @oli-obk's comment in #85828 (comment)

For things like if const { expr } { 0 } else { do something complex } it'd be nice to not have to even give LLVM the code for the else branch to have to emit. Ought to save time and binary size.

Tentatively marked mir-opt, since we presumably still need to make the MIR for it, in order to borrow-check and such the unreachable code. But might need to be codegen-time instead to pick up on monomorphized inline constants.

Hopefully should be doable relatively easily when the const is an inline const, since that forces const folding already, and thus there should just be a const true in the MIR.

@scottmcm scottmcm added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-mir-opt Area: MIR optimizations F-inline_const Inline constants (aka: const blocks, const expressions, anonymous constants) and removed A-mir-opt Area: MIR optimizations labels May 30, 2021
@scottmcm
Copy link
Member Author

scottmcm commented May 30, 2021

Well, turns out this already works for simple constants in MIR! 🎉
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=4d1fe08eec8b705525535ec9ec9d1c89

However, the place I actually needed it was more like if const { size_of::<T>() < 100 }, which doesn't work with inline_const today. So maybe come back to this once that's fixed.
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=80c275a75dc280ac3fcdfb24b6484adc

@nagisa
Copy link
Member

nagisa commented May 30, 2021

This should be done by SimplifyBranches which AFAIK is run unconditionally. I wonder if its match needs to be expanded?

Is there mcve?

@nagisa nagisa added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label May 30, 2021
@oli-obk
Copy link
Contributor

oli-obk commented May 30, 2021

The problem is that this can't be done on generic constants, as we don't mir-optimize post-monomorphization. We could start doing some benchmarks to see how bad running all mir opts (or at least a select few) would be if we duplicated, substituted and optimized MIR bodies for concrete generic parameters.

@oli-obk
Copy link
Contributor

oli-obk commented May 30, 2021

I would say

trait SizeBiggerFour: Sized {
    const VALUE: bool = std::mem::size_of::<Self>() > 4;
}

impl<T> SizeBiggerFour for T {}

pub fn foo<T>() -> i32 { if <T as SizeBiggerFour>::VALUE { 3 } else { 5 } }
pub fn bar() -> i32 { foo::<isize>() }

qualifies as an MCVE as the monomorphized foo::<isize> gives the following llvm ir

; playground::foo
; Function Attrs: nonlazybind uwtable
define i32 @_ZN10playground3foo17hb9992b01cb0c258eE() unnamed_addr #0 !dbg !6 {
start:
  %0 = alloca i32, align 4
  br i1 true, label %bb1, label %bb2, !dbg !15

bb2:                                              ; preds = %start
  store i32 5, i32* %0, align 4, !dbg !16
  br label %bb3, !dbg !15

bb1:                                              ; preds = %start
  store i32 3, i32* %0, align 4, !dbg !17
  br label %bb3, !dbg !15

bb3:                                              ; preds = %bb2, %bb1
  %1 = load i32, i32* %0, align 4, !dbg !18
  ret i32 %1, !dbg !18
}

@the8472
Copy link
Member

the8472 commented May 30, 2021

We could start doing some benchmarks to see how bad running all mir opts (or at least a select few) would be if we duplicated, substituted and optimized MIR bodies for concrete generic parameters.

Maybe it should also be evaluated together with polymorphization or const_evaluatable_checked since those can cut down on the duplication before applying further optimizations.

@bjorn3 bjorn3 removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label May 30, 2021
@scottmcm
Copy link
Member Author

scottmcm commented Jun 1, 2021

Bonus points if this can avoid monomorphizing functions called in the dead code too.

Demo extracted from core: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=40cc11066ca1fa8462d0f0f403ee1d8f (cc #52051)

Would be great to not codegen all the downstream stuff that's only called in the else block of a br i1 true.

@nbdd0121
Copy link
Contributor

nbdd0121 commented May 1, 2022

However, the place I actually needed it was more like if const { size_of::<T>() < 100 }, which doesn't work with inline_const today. So maybe come back to this once that's fixed.

FYI this should work with #96557

@DemiMarie

This comment was marked as off-topic.

@scottmcm

This comment was marked as off-topic.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 25, 2022
…=thomcc

Make ZST checks in core/alloc more readable

There's a bunch of these checks because of special handing for ZSTs in various unsafe implementations of stuff.

This lets them be `T::IS_ZST` instead of `mem::size_of::<T>() == 0` every time, making them both more readable and more terse.

*Not* proposed for stabilization.  Would be `pub(crate)` except `alloc` wants to use it too.

(And while it doesn't matter now, if we ever get something like rust-lang#85836 making it a const can help codegen be simpler.)
@RalfJung
Copy link
Member

This seems closely related to rust-lang/rfcs#3582 and #122301.

@RalfJung
Copy link
Member

RalfJung commented Mar 25, 2024

@saethlin's recent PR #121421 seems related here? If the if condition becomes a constant during codegen, we should be entirely skipping it now and not even generate LLVM IR.

However, if that dead code calls functions, those still get codegen'd as usual -- the collector that determines which function to codegen doesn't know about dead branches. We'd have to embed a (simple) monomorphization-time reachability analysis in the collector to fix that.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 31, 2024
…try>

Only collect mono items from reachable blocks

Fixes the wrong commented pointed out in: rust-lang#121421 (comment)
Moves the analysis to use the worklist strategy: rust-lang#121421 (comment)
Also fixes rust-lang#85836, using the same reachability analysis
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 1, 2024
…try>

Only collect mono items from reachable blocks

Fixes the wrong commented pointed out in: rust-lang#121421 (comment)
Moves the analysis to use the worklist strategy: rust-lang#121421 (comment)
Also fixes rust-lang#85836, using the same reachability analysis
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 1, 2024
…try>

Only collect mono items from reachable blocks

Fixes the wrong commented pointed out in: rust-lang#121421 (comment)
Moves the analysis to use the worklist strategy: rust-lang#121421 (comment)
Also fixes rust-lang#85836, using the same reachability analysis
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 2, 2024
…try>

Only collect mono items from reachable blocks

Fixes the wrong commented pointed out in: rust-lang#121421 (comment)
Moves the analysis to use the worklist strategy: rust-lang#121421 (comment)
Also fixes rust-lang#85836, using the same reachability analysis
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 2, 2024
…try>

Only collect mono items from reachable blocks

Fixes the wrong commented pointed out in: rust-lang#121421 (comment)
Moves the analysis to use the worklist strategy: rust-lang#121421 (comment)
Also fixes rust-lang#85836, using the same reachability analysis
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 2, 2024
…try>

Only collect mono items from reachable blocks

Fixes the wrong commented pointed out in: rust-lang#121421 (comment)
Moves the analysis to use the worklist strategy: rust-lang#121421 (comment)
Also fixes rust-lang#85836, using the same reachability analysis
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 7, 2024
…jgillot

Only collect mono items from reachable blocks

Fixes the wrong comment pointed out in: rust-lang#121421 (comment)
Moves the analysis to use the worklist strategy: rust-lang#121421 (comment)
Also fixes rust-lang#85836, using the same reachability analysis
@bors bors closed this as completed in bb78dba Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. F-inline_const Inline constants (aka: const blocks, const expressions, anonymous constants)
Projects
None yet
8 participants