Simplify `PartialOrd` on tuples containing primitives #138135

scottmcm · 2025-03-07T00:36:08Z

We noticed in #133984 (comment) that currently the tuple comparison code, while it does optimize down today, is kinda huge: https://rust.godbolt.org/z/xqMoeYbhE

This PR changes the tuple code to go through an overridable "chaining" version of the comparison functions, so that for simple things like (i16, u16) and (f32, f32) (as seen in the new MIR pre-codegen test) we just directly get the

if lhs.0 == rhs.0 { lhs.0 OP rhs.0 }
else { lhs.1 OP rhs.1 }

version in MIR, rather than emitting a mess for LLVM to have to clean up.

Test added in the first commit, so you can see the MIR diff in the second one.

scottmcm · 2025-03-07T00:48:00Z

tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir

+        StorageDead(_3);
+        _9 = &((*_1).1: f32);
+        _10 = &((*_2).1: f32);
+        _0 = <f32 as PartialOrd>::le(move _9, move _10) -> [return: bb3, unwind continue];


...well, it almost optimizes down completely. At least it's down to just 4 blocks, which is the smallest we can have here since (AFAIK) we're not ~~allowed~~ supposed to have two returns.

I wasn't sure if we had something tracking this inliner imperfection, so filed #138136 to have a way to reference it.

And actually in the 2 weeks since I opened this I managed to fix that inliner imperfection, so the MIR here is now really good -- no function calls left at all 🎉

(Though it still has the #138544 problem, but that's much more minor in comparison.)

…<try> Allow more top-down inlining for single-BB callees This means that things like `<usize as Step>::forward_unchecked` and `<PartialOrd for f32>::le` will inline even if we've already done a bunch of inlining to find the calls to them. Fixes rust-lang#138136 Draft as it's built atop rust-lang#138135, which adds a mir-opt test that's a nice demonstration of this.

…<try> Allow more top-down inlining for single-BB callees This means that things like `<usize as Step>::forward_unchecked` and `<PartialOrd for f32>::le` will inline even if we've already done a bunch of inlining to find the calls to them. Fixes rust-lang#138136 ~~Draft as it's built atop rust-lang#138135, which adds a mir-opt test that's a nice demonstration of this. To see just this change, look at <https://github.com/rust-lang/rust/pull/138157/commits/48f63e3be552605c2933056b77bf23a326757f92>~~ Rebased to be just the inlining change, as the other existing tests show it great.

…oli-obk Allow more top-down inlining for single-BB callees This means that things like `<usize as Step>::forward_unchecked` and `<PartialOrd for f32>::le` will inline even if we've already done a bunch of inlining to find the calls to them. Fixes rust-lang#138136 ~~Draft as it's built atop rust-lang#138135, which adds a mir-opt test that's a nice demonstration of this. To see just this change, look at <https://github.com/rust-lang/rust/pull/138157/commits/48f63e3be552605c2933056b77bf23a326757f92>~~ Rebased to be just the inlining change, as the other existing tests show it great.

We have codegen ones, but it looks like we could make those less flakey by just doing something better in the first place...

scottmcm · 2025-03-19T16:30:01Z

tests/mir-opt/pre-codegen/tuple_ord.rs

@@ -12,5 +12,5 @@ pub fn demo_le_total(a: &(u16, i16), b: &(u16, i16)) -> bool {
 // EMIT_MIR tuple_ord.demo_ge_partial.PreCodegen.after.mir
 pub fn demo_ge_partial(a: &(f32, f32), b: &(f32, f32)) -> bool {
    // CHECK-LABEL: demo_ge_partial
-    a <= b
+    a >= b


Doh, I somehow managed to not notice that I'd put <= in the ge test 🤦

Sorry for the slightly-worse diff; it doesn't really change anything material though.

…oli-obk Allow more top-down inlining for single-BB callees This means that things like `<usize as Step>::forward_unchecked` and `<PartialOrd for f32>::le` will inline even if we've already done a bunch of inlining to find the calls to them. Fixes rust-lang#138136 ~~Draft as it's built atop rust-lang#138135, which adds a mir-opt test that's a nice demonstration of this. To see just this change, look at <https://github.com/rust-lang/rust/pull/138157/commits/48f63e3be552605c2933056b77bf23a326757f92>~~ Rebased to be just the inlining change, as the other existing tests show it great.

scottmcm · 2025-03-21T05:55:00Z

r? libs

Mark-Simulacrum · 2025-03-23T16:26:14Z

library/core/src/cmp.rs

+/// directly, instead of needing to optimize the 3-way comparison.
+///
+/// Currently this is done using specialization, but it doesn't need that:
+/// it could be provided methods on `PartialOrd` instead and work fine.


Is it worse for compile times or similar to make these (unstable) provided methods? If we can avoid another use of specialization that seems worthwhile to me - I forget if core's usage is guaranteed sound or not (I seem to recall some gaps)...

I think this usage is sound, since we're only specializing on primitives that don't have lifetimes. But I was torn between the two anyway, so if you have a weak preference for the other way I'm happy to give that a shot. Let's see how it comes out. I always like less specialization 🙂

@rustbot author

Mark-Simulacrum · 2025-03-23T16:30:06Z

tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir

+        StorageDead(_4);
+        StorageDead(_3);
+        _8 = copy ((_7 as Break).0: bool);
+        _0 = copy _8;


I'm a bit surprised we're not able to make this block _0 = Ge(...) like bb2 ends up as... I'm sure LLVM will work it out though.

I guess this is #138544 :)

Yup, that's right. bb2 is the second field, so it's just return a.1 < b.1, and doesn't have anything to optimize out, but here in bb1 we can't quite fix it in MIR yet became the passes that know how to fix it don't see it in the form we see here in the PreCodegen MIR -- earlier before another round of SimplifyCfg the basic block structure is messier.

And yes, LLVM will fix it. I'm also working on other changes (#138759 and the unfinished #138582) that'll mean it'll even be fixed in debug codegen, rather than SRoA needing to fix it in LLVM.

Mark-Simulacrum · 2025-03-23T16:31:05Z

r=me if refactoring to avoid specialization doesn't seem warranted to you, not a strong opinion there.

Uses `__`-named `doc(hidden)` methods instead.

scottmcm · 2025-03-23T23:37:06Z

Nice, I like this better. I think it'd fix more easily with extending it to other things too, though I'm not going to do that in this PR.

@bors r=Mark-Simulacrum

bors · 2025-03-23T23:37:09Z

📌 Commit 7781346 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

rustbot assigned workingjubilee Mar 7, 2025

rustbot added S-waiting-on-review T-compiler T-libs labels Mar 7, 2025

This comment has been minimized.

Sign in to view

scottmcm commented Mar 7, 2025

View reviewed changes

scottmcm force-pushed the chaining-ord branch from bf1c98f to de4e4a3 Compare March 7, 2025 00:59

This was referenced Mar 7, 2025

Lower BinOp::Cmp to llvm.{s,u}cmp.* intrinsics #133984

Open

Allow more top-down inlining for single-BB callees #138157

Merged

Add a MIR pre-codegen test for tuple comparisons

b54ca0e

We have codegen ones, but it looks like we could make those less flakey by just doing something better in the first place...

scottmcm force-pushed the chaining-ord branch from de4e4a3 to d6c3e89 Compare March 19, 2025 16:21

Add chaining versions of lt/le/gt/ge and use them in tuple PartialOrd

35248c6

scottmcm force-pushed the chaining-ord branch from d6c3e89 to 35248c6 Compare March 19, 2025 16:27

scottmcm commented Mar 19, 2025

View reviewed changes

rustbot assigned Mark-Simulacrum and unassigned workingjubilee Mar 21, 2025

Mark-Simulacrum reviewed Mar 23, 2025

View reviewed changes

rustbot added S-waiting-on-author and removed S-waiting-on-review labels Mar 23, 2025

Stop using specialization for this

7781346

Uses `__`-named `doc(hidden)` methods instead.

bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify `PartialOrd` on tuples containing primitives #138135

Simplify `PartialOrd` on tuples containing primitives #138135

scottmcm commented Mar 7, 2025 •

edited

Loading

This comment has been minimized.

scottmcm Mar 7, 2025 •

edited

Loading

scottmcm Mar 19, 2025

scottmcm Mar 19, 2025

scottmcm commented Mar 21, 2025

Mark-Simulacrum Mar 23, 2025

scottmcm Mar 23, 2025 •

edited

Loading

Mark-Simulacrum Mar 23, 2025

Mark-Simulacrum Mar 23, 2025

scottmcm Mar 23, 2025

Mark-Simulacrum commented Mar 23, 2025

scottmcm commented Mar 23, 2025

bors commented Mar 23, 2025

Simplify PartialOrd on tuples containing primitives #138135

Are you sure you want to change the base?

Simplify PartialOrd on tuples containing primitives #138135

Conversation

scottmcm commented Mar 7, 2025 • edited Loading

This comment has been minimized.

scottmcm Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

scottmcm Mar 19, 2025

Choose a reason for hiding this comment

scottmcm Mar 19, 2025

Choose a reason for hiding this comment

scottmcm commented Mar 21, 2025

Mark-Simulacrum Mar 23, 2025

Choose a reason for hiding this comment

scottmcm Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

Mark-Simulacrum Mar 23, 2025

Choose a reason for hiding this comment

Mark-Simulacrum Mar 23, 2025

Choose a reason for hiding this comment

scottmcm Mar 23, 2025

Choose a reason for hiding this comment

Mark-Simulacrum commented Mar 23, 2025

scottmcm commented Mar 23, 2025

bors commented Mar 23, 2025

Simplify `PartialOrd` on tuples containing primitives #138135

Simplify `PartialOrd` on tuples containing primitives #138135

scottmcm commented Mar 7, 2025 •

edited

Loading

scottmcm Mar 7, 2025 •

edited

Loading

scottmcm Mar 23, 2025 •

edited

Loading