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.

Sign up for GitHub

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

Jump to bottom

MIR inliner not inlining through is_some_and sometimes #137901

Closed
scottmcm opened this issue Mar 2, 2025 · 1 comment · Fixed by #137907
Closed

MIR inliner not inlining through is_some_and sometimes #137901

scottmcm opened this issue Mar 2, 2025 · 1 comment · Fixed by #137907
Labels
A-mir-opt-inlining Area: MIR inlining

Comments

@scottmcm
Copy link
Member

scottmcm commented Mar 2, 2025

Noticed in 2025-03-01 (which at time of writing isn't on godbolt yet) that the MIR for <= for a generated type still contains

    bb1: {
        _4 = move ((_3 as Some).0: std::cmp::Ordering);
        StorageLive(_5);
        _5 = (copy _4,);
        _0 = <fn(std::cmp::Ordering) -> bool {std::cmp::Ordering::is_le} as FnOnce<(std::cmp::Ordering,)>>::call_once(std::cmp::Ordering::is_le, move _5) -> [return: bb2, unwind continue];
    }

https://play.rust-lang.org/?version=nightly&mode=release&edition=2024&gist=57c2829c1cf0a8778926bd87a0378feb

I can workaround that particular case, but calls to trivial functions like is_le really ought to MIR-inline, even if done through the voldemort function item like this.
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 2, 2025
@scottmcm scottmcm 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 Mar 2, 2025
@scottmcm scottmcm changed the title MIR inliner not inling through is_some_and sometimes MIR inliner not inlining through is_some_and sometimes Mar 2, 2025
@scottmcm scottmcm mentioned this issue Mar 2, 2025
Merged
@scottmcm
Copy link
Member Author

scottmcm commented Mar 2, 2025

Weirdly, this seems to be due to using .is_some_and(Ordering::is_le) -- changing it to .is_some_and(|c| c.is_le()) makes it inline again 🤯

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 3, 2025
@bors
Auto merge of rust-lang#137904 - scottmcm:ordering-is, r=<try>
5c944f0 
Improve the generic MIR in the default `PartialOrd::le` and friends

It looks like I regressed this accidentally in rust-lang#137197 due to rust-lang#137901

So this PR does two things:
1. Tweaks the way we're calling `is_some_and` so that it optimizes in the generic MIR (rather than needing to optimize it in every monomorphization) -- the first commit adds a MIR test, so you can see the difference in the second commit.
2. Updates the implementations of `is_le` and friends to be slightly simpler, and parallel how clang does them.
@compiler-errors compiler-errors mentioned this issue Mar 3, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 3, 2025
@bors
Auto merge of rust-lang#137907 - compiler-errors:inline-fnonce, r=<try>
66b5f9d 
Inline `FnOnce`/`FnMut`/`Fn` shims once again

cc rust-lang#137901

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 3, 2025
@bors
Auto merge of rust-lang#137907 - compiler-errors:inline-fnonce, r=<try>
633435e 
Inline `FnOnce`/`FnMut`/`Fn` shims once again

cc rust-lang#137901

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 3, 2025
@bors
Auto merge of rust-lang#137907 - compiler-errors:inline-fnonce, r=<try>
76aedcd 
Inline `FnOnce`/`FnMut`/`Fn` shims once again

This PR fixes the argument checking for `extern "rust-call"` ABI functions with a spread arg, which do no expect their arguments to be exploded from a tuple like closures do.

Secondly, it removes the hack that prevented them from being inlined. This results in more work done by the compiler, but it does end up allowing us to inline functions we didn't before.

Fixes rust-lang#137901
@bors bors closed this as completed in 30f168e Mar 5, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Mar 7, 2025
@workingjubilee
Rollup merge of rust-lang#137904 - scottmcm:ordering-is, r=workingjub…
a727e02 
…ilee

Improve the generic MIR in the default `PartialOrd::le` and friends

It looks like I regressed this accidentally in rust-lang#137197 due to rust-lang#137901

So this PR does two things:
1. Tweaks the way we're calling `is_some_and` so that it optimizes in the generic MIR (rather than needing to optimize it in every monomorphization) -- the first commit adds a MIR test, so you can see the difference in the second commit.
2. Updates the implementations of `is_le` and friends to be slightly simpler, and parallel how clang does them.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 7, 2025
@matthiaskrgr
Rollup merge of rust-lang#137904 - scottmcm:ordering-is, r=workingjub…
9e16082 
…ilee

Improve the generic MIR in the default `PartialOrd::le` and friends

It looks like I regressed this accidentally in rust-lang#137197 due to rust-lang#137901

So this PR does two things:
1. Tweaks the way we're calling `is_some_and` so that it optimizes in the generic MIR (rather than needing to optimize it in every monomorphization) -- the first commit adds a MIR test, so you can see the difference in the second commit.
2. Updates the implementations of `is_le` and friends to be slightly simpler, and parallel how clang does them.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 7, 2025
@rust-timer
Unrolled build for rust-lang#137904
e87d9fc 
Rollup merge of rust-lang#137904 - scottmcm:ordering-is, r=workingjubilee

Improve the generic MIR in the default `PartialOrd::le` and friends

It looks like I regressed this accidentally in rust-lang#137197 due to rust-lang#137901

So this PR does two things:
1. Tweaks the way we're calling `is_some_and` so that it optimizes in the generic MIR (rather than needing to optimize it in every monomorphization) -- the first commit adds a MIR test, so you can see the difference in the second commit.
2. Updates the implementations of `is_le` and friends to be slightly simpler, and parallel how clang does them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-mir-opt-inlining Area: MIR inlining
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Inline FnOnce/FnMut/Fn shims once again compiler-errors/rust
2 participants
@scottmcm @rustbot