[bushrat011899]

View all comments

Tracking Issue: #160728

Description

Based on this feedback from the initial implementation, I've added a mir-opt test to highlight the amount of MIR added by using a variadic min implementation. To my knowledge, there isn't a way to actually test the MIR generated by the actual implementation in core, so I've replicated a smaller representative sample in a test instead. Please see GodBolt for a more detailed comparison between the (unoptimised) MIR for the current and splatted versions of min (max omitted since it's identical).

In short, the splat version does produce more MIR, especially for the additional trait implementations for larger tuple sizes. However, the added MIR almost exclusively comes from the indirection of calling a method on a trait, and not specifically the #[rustc_splat] variadic implementation. Because of this, it may be worth considering a MIR pass which could inline this kind of static dispatch.

Moving to a pre-codegen test with some MIR optimisation has narrowed the gap between smallest and min to basically nothing (an extra scope from the looks of it). I'm guessing the chained a.min(b).min(c).min(d) style (with a lack of explicit temporaries) is something MIR is able to readily optimise!

From experimenting with this test, I also found a simpler way to implement smaller and larger. The most notable difference is instead of passing all arguments through the splat, I've pulled out the first argument so they aren't de/tupled. It improves codegen slightly, but more importantly simplifies the macro implementation.

---

Notes

• No AI tooling of any kind was used during the creation of this PR.

[rustbot]

r? @JonathanBrouwer

rustbot has assigned @JonathanBrouwer.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 16 candidates

[JonathanBrouwer]

r? libs

[Mark-Simulacrum]

show that at the rust level it folds down to the same thing

I'm wondering if it would be easier/better to show this on -Cno-prepopulate-passes LLVM IR? That should be able to do it without duplicating core's impl, as long as the impls in core are #[inline]d into the calling code. (I assume they would be since it's all generic code even without any explicit inlines?).

cc @scottmcm

Looking at the actual impl here, I guess the reason we're pulling out two tuple 'parts' is to simplify the common case of having smallest/largest with just two arguments passed? I'm wondering if we'd get better codegen in debug mode if we made this more explicitly a loop, by transmuting the (T, T, ...) into [T; N] and then going through Iterator::{min, max}? I'm pretty sure we don't guarantee enough about repr(Rust) to make that transmute always compile, but if it does, then I think it is a sound way to access the elements?

In particular I suspect doing it via min/max on the array makes it easier for LLVM to notice the loop and not lower to "forcefully" unrolled code... while hopefully not making the common (short) case much worse.

[scottmcm]

I think what we probably want here is a pre-codegen MIR test: https://github.com/rust-lang/rust/tree/main/tests/mir-opt/pre-codegen

Notably, those do run mir optimizations, so it should be fine to call cmp::min and cmp::smallest and see what they do. (Which, after #161081, should both just be "call the compiler intrinsic.)

If open-coded smallest will have more overhead in unoptimized, but I'm not actually that worried about it. We always ship the sysroot with mir optimizations, for example.

I mostly just want to see that the various method layers properly fold away thanks to the mir-opt-level=2 when people call the std methods. (This is similar to the tests that make sure that passing tiny closures to Option::map can get inlined so it's not gratuitously different from a manual if-let.)

---

TBH it might not even need any changes to the implementation, since https://rust.godbolt.org/z/cGPqao9jf is already looking pretty reasonable. (That nightly doesn't have the min intrinsic, though, so you'll see in a test here will probably be different.)

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[scottmcm]

I'm wondering if we'd get better codegen in debug mode if we made this more explicitly a loop

My instinct is that that'd be really really bad for the common cases, since we'd end up with the loop in MIR even for the 2-param case, which then wouldn't fold down (mir-opt doesn't have loop optimizations) and thus wouldn't be inlinable and would thus be overall worse.

To me by the time you're passing the N args separately, it's fine (maybe even good) to forcibly expand to N calls.

(Said otherwise, I think of this as "you don't have to write min(min(a, b), min(c, d)) any more", not as a replacement for [a, b, c, d].into_iter().min().unwrap().)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[bushrat011899]

Looking at the actual impl here, I guess the reason we're pulling out two tuple 'parts' is to simplify the common case of having smallest/largest with just two arguments passed?

Correct, however it appears to be of little benefit in hindsight. I'm going to follow Scottmcm's suggestion and only split out a single value.

I'm wondering if we'd get better codegen in debug mode if we made this more explicitly a loop, by transmuting the (T, T, ...) into [T; N] and then going through Iterator::{min, max}? I'm pretty sure we don't guarantee enough about repr(Rust) to make that transmute always compile, but if it does, then I think it is a sound way to access the elements?

I did experiment with this on Godbolt a while ago and didn't really see any benefits. And the lack of a layout guarantee does make it a dubious optimisation. If it were valid, I'd expect to see it in the From<(T, T, ..)> for [T; N] implementation.

I think what we probably want here is a pre-codegen MIR test: https://github.com/rust-lang/rust/tree/main/tests/mir-opt/pre-codegen. Notably, those do run mir optimizations, so it should be fine to call cmp::min and cmp::smallest and see what they do. (Which, after #161081, should both just be "call the compiler intrinsic.)

Ahh that's much better. Thanks for the pointer! I've simplified the test to actually call into std::cmp rather than copying the code.

[bushrat011899]

@rustbot ready

[scottmcm]

This is mostly looking good, but I did see a few small things that I think would make sense to address.

View changes since this review

[scottmcm]

Thanks for this! Makes me way more confident that we'll be able to migrate to these since even early we're already able to eliminate the indirections.

@bors r+

[rust-bors]

📌 Commit d0706de has been approved by scottmcm

It is now in the queue for this repository.