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

Range<usize>::next should fully MIR-inline #130590

Closed
scottmcm opened this issue Sep 20, 2024 · 2 comments · Fixed by #138157
Closed

Range<usize>::next should fully MIR-inline #130590

scottmcm opened this issue Sep 20, 2024 · 2 comments · Fixed by #138157
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt-inlining Area: MIR inlining C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

If you compile this to optimized MIR:

// @compile-flags: -O -C debuginfo=0 --emit=mir -Z inline-mir-threshold=9999 -Z inline-mir-hint-threshold=9999
use std::ops::Range;
#[no_mangle]
pub fn demo(num: &mut Range<usize>) -> Option<usize> {
    num.next()
}

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

You'll see that it still contains a call to forward_unchecked:

    bb1: {
        _3 = copy ((*_1).0: usize);
        StorageLive(_4);
        _4 = <usize as Step>::forward_unchecked(copy _3, const 1_usize) -> [return: bb2, unwind continue];
    }

That's pretty unfortunate, because forward_unchecked(a, 1) is just AddUnchecked(a, 1), a single MIR operator.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 20, 2024
@scottmcm
Copy link
Member Author

scottmcm commented Sep 20, 2024

I tried adding #[inline] to the default step methods just in case that was relevant, but it looks like it wasn't.

Maybe something to do with specialization since I think this ends up at

#[inline]
fn spec_next(&mut self) -> Option<T> {
if self.start < self.end {
let old = self.start;
// SAFETY: just checked precondition
self.start = unsafe { Step::forward_unchecked(old, 1) };
Some(old)
} else {
None
}
}

EDIT: Also tried increasing

const TOP_DOWN_DEPTH_LIMIT: usize = 5;

and that wasn't enough either.

@lolbinarycat lolbinarycat added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such labels Sep 20, 2024
@jieyouxu jieyouxu added A-mir-opt-inlining Area: MIR inlining and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 20, 2024
@scottmcm
Copy link
Member Author

this fixes it, but also breaks the exponential growth test, so needs something smarter...

         let inline_limit = match self.history.len() {
             0 => usize::MAX,
-            1..=TOP_DOWN_DEPTH_LIMIT => 1,
+            1..=TOP_DOWN_DEPTH_LIMIT => 3,
             _ => return,
         };

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 22, 2024
Inline smaller callees first

Then limit the total size and number of inlined things, to allow more top-down inlining (particularly important after calling something generic that couldn't inline internally) without getting exponential blowup.

Fixes rust-lang#130590
r? saethlin
@scottmcm scottmcm linked a pull request Mar 11, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt-inlining Area: MIR inlining C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
4 participants