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

scottmcm · 2025-03-02T22:55:15Z

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

So this PR does two things:

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. Thanks to some quick work from C-E this became no longer necessary.
Updates the implementations of is_le and friends to be slightly simpler, and parallel how clang does them.

rustbot · 2025-03-02T22:55:20Z

rustbot has assigned @workingjubilee.
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

scottmcm · 2025-03-02T23:04:30Z

tests/mir-opt/pre-codegen/derived_ord.demo_le.PreCodegen.after.mir

+        _12 = copy ((_11 as Some).0: std::cmp::Ordering);
+        StorageLive(_13);
+        _13 = discriminant(_12);
+        _0 = Le(move _13, const 0_i8);


Using this implementation for is_le means that we get an Le in the MIR (and similarly is_gt gives a Gt, etc)

The previous matches! implementation meant that this was

_14 = Eq(copy _13, const 1_i8); _0 = Not(move _14);

which is much less obviously a <=.

(That should also make these look like what you'd get in C++ for things like (a <=> b) <= 0, https://en.cppreference.com/w/cpp/utility/compare/strong_ordering#Comparisons, and thus make LLVM more likely to continue getting them right since it's a code pattern that clang uses too:
https://github.com/llvm/llvm-project/blob/60486292b79885b7800b082754153202bef5b1f0/libcxx/include/__compare/is_eq.h#L26 )

Huh. Do we have a MIR transform that would normalize that...?

No, because we don't have an InstCombine system.

GVN can't do it for two reasons:

_11 has two initializers, so it's ignored by all SsaLocals-based transforms.

There's no _n = Le(…, …) in the program, and GVN can only replace things with locals or constants, not new never-seen-before Rvalues.

EDIT: err, I replied about making a Le, but you probably meant why we can't it can't be _0 = Ne(copy _13, const 1_i8). So only the latter reason applies.

scottmcm · 2025-03-03T00:56:42Z

@bors try @rust-timer queue

bors · 2025-03-03T00:57:53Z

⌛ Trying commit fcb91b8 with merge 5c944f0...

workingjubilee · 2025-03-03T00:57:59Z

tests/mir-opt/pre-codegen/derived_ord.demo_le.PreCodegen.after.mir

-        _13 = (copy _12,);
-        _0 = <fn(std::cmp::Ordering) -> bool {std::cmp::Ordering::is_le} as FnOnce<(std::cmp::Ordering,)>>::call_once(std::cmp::Ordering::is_le, move _13) -> [return: bb4, unwind unreachable];
-    }


workingjubilee · 2025-03-03T00:59:14Z

tests/mir-opt/pre-codegen/derived_ord.demo_le.PreCodegen.after.mir

+        _12 = copy ((_11 as Some).0: std::cmp::Ordering);
+        StorageLive(_13);
+        _13 = discriminant(_12);
+        _0 = Le(move _13, const 0_i8);


Huh. Do we have a MIR transform that would normalize that...?

workingjubilee · 2025-03-03T01:02:50Z

weird.
@bors r+

bors · 2025-03-03T01:02:53Z

📌 Commit fcb91b8 has been approved by workingjubilee

It is now in the queue for this repository.

compiler-errors · 2025-03-03T01:24:06Z

@bors r-

pls do not r+ a try build

compiler-errors · 2025-03-03T01:24:33Z

Wait until rust-lang-ci@5c944f0 is done

compiler-errors · 2025-03-03T01:25:19Z

Also probably wait until perf is done too? Doesn't seem like there's a good reason to merge something that attempts to fix perf without knowing if it actually fixes perf?

workingjubilee · 2025-03-03T01:32:57Z

@compiler-errors Oh, sorry, didn't see the try.

bors · 2025-03-03T02:58:57Z

☀️ Try build successful - checks-actions
Build commit: 5c944f0 (5c944f0112f9b8303f412ed005600d553c4e8b34)

rust-timer · 2025-03-03T04:42:31Z

Finished benchmarking commit (5c944f0): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	6
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.3%]	3
All ❌✅ (primary)	-0.1%	[-0.4%, 0.8%]	7

Max RSS (memory usage)

Results (primary 0.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	7.0%	[3.1%, 12.2%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-10.1%, -1.7%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[-10.1%, 12.2%]	6

Cycles

Results (primary 0.8%, secondary -3.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-3.5%, -3.5%]	1
All ❌✅ (primary)	0.8%	[0.8%, 0.8%]	1

Binary size

Results (primary -0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.6%]	9
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	32
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.2%, 0.6%]	41

Bootstrap: 773.437s -> 771.311s (-0.27%)
Artifact size: 361.95 MiB -> 361.93 MiB (-0.01%)

workingjubilee · 2025-03-03T05:11:00Z

hm. the regression specifically on cargo seems odd to me.

scottmcm · 2025-03-03T05:17:28Z

This looks like it's basically undoing the changes from #137796 (comment), as expected. Cargo improved in that one, worsened in this one. Cranelift, nalgebra, and webrender all regressed in doc in that one, then improved in this one.

But because this affects mir it affects inlining, so I think the cargo change is mostly in churn -- the codegen schedule is changed up, not just scaled by 1%.

You can also see the effect of the better generic MIR for this in the bunch of debug cases that get smaller (by a very small amount) in binary size, without other perf impact to those cases.

So unless you're opposed, @workingjubilee, I'm inclined to merge under #137904 (comment)

scottmcm · 2025-03-05T17:41:26Z

Since c-e so quickly got #137907 working, I think I'll
@bors r-
and merge a simpler version of this after that one means I don't need one of the workarounds.

scottmcm · 2025-03-06T20:34:57Z

This is probably perf-neutral now since #137907 got the improvement in the default PartialOrd methods, so
@bors r=workingjubilee rollup=maybe

(This MIR test will also be good for future changes, since for example it has an Option that's only ever Some, which would be a good thing to optimize away.)

bors · 2025-03-06T20:34:59Z

📌 Commit eae5ed6 has been approved by workingjubilee

It is now in the queue for this repository.

scottmcm · 2025-03-09T23:40:38Z

No icount change in the rollup, as expected, but something was still a size change, so I'm curious

@rust-timer build e87d9fc

rust-timer · 2025-03-10T01:28:09Z

Finished benchmarking commit (e87d9fc): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.3%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.5%, secondary -3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.4%	[1.9%, 6.8%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-4.0%, -2.3%]	4
Improvements ✅ (secondary)	-3.2%	[-3.2%, -3.2%]	1
All ❌✅ (primary)	-0.5%	[-4.0%, 6.8%]	6

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.2%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.1%, -0.0%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.1%, 0.2%]	9

Bootstrap: 765.289s -> 764.761s (-0.07%)
Artifact size: 362.07 MiB -> 362.10 MiB (0.01%)

rustbot assigned workingjubilee Mar 2, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 2, 2025

scottmcm commented Mar 2, 2025

View reviewed changes

scottmcm force-pushed the ordering-is branch 2 times, most recently from 4c41a72 to fcb91b8 Compare March 2, 2025 23:33

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 3, 2025

workingjubilee approved these changes Mar 3, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2025

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 3, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 3, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 3, 2025

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 5, 2025

scottmcm closed this Mar 5, 2025

scottmcm reopened this Mar 5, 2025

scottmcm added 2 commits March 5, 2025 21:45

Also add a MIR pre-codegen test for the derived PartialOrd::le

1b21952

Make is_le and friends work like clang's

eae5ed6

scottmcm force-pushed the ordering-is branch from fcb91b8 to eae5ed6 Compare March 6, 2025 06:08

scottmcm closed this Mar 6, 2025

scottmcm reopened this Mar 6, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 6, 2025

workingjubilee mentioned this pull request Mar 7, 2025

Rollup of 12 pull requests #138146

Closed

matthiaskrgr mentioned this pull request Mar 7, 2025

Rollup of 8 pull requests #138151

Merged

bors merged commit 9e16082 into rust-lang:master Mar 7, 2025
9 of 12 checks passed

rustbot added this to the 1.87.0 milestone Mar 7, 2025

scottmcm deleted the ordering-is branch March 8, 2025 05:03

This comment has been minimized.

Sign in to view

Improve the generic MIR in the default PartialOrd::le and friends #137904

Improve the generic MIR in the default PartialOrd::le and friends #137904

Uh oh!

Conversation

scottmcm commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm commented Mar 3, 2025

Uh oh!

This comment has been minimized.

bors commented Mar 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented Mar 3, 2025

Uh oh!

bors commented Mar 3, 2025

Uh oh!

compiler-errors commented Mar 3, 2025

Uh oh!

compiler-errors commented Mar 3, 2025

Uh oh!

compiler-errors commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

workingjubilee commented Mar 3, 2025

Uh oh!

bors commented Mar 3, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Mar 3, 2025

Overall result: ✅ improvements - no action needed

Uh oh!

workingjubilee commented Mar 3, 2025

Uh oh!

scottmcm commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmcm commented Mar 5, 2025

Uh oh!

scottmcm commented Mar 6, 2025

Uh oh!

bors commented Mar 6, 2025

Uh oh!

Uh oh!

scottmcm commented Mar 9, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Mar 10, 2025

Overall result: ❌ regressions - no action needed

Uh oh!

Uh oh!

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

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

scottmcm commented Mar 2, 2025 •

edited

Loading

scottmcm Mar 3, 2025 •

edited

Loading

compiler-errors commented Mar 3, 2025 •

edited

Loading

scottmcm commented Mar 3, 2025 •

edited

Loading