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

[scottmcm]

It looks like I regressed this accidentally in #137197 due to #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. Thanks to some quick work from C-E this became no longer necessary.
2. Updates the implementations of is_le and friends to be slightly simpler, and parallel how clang does them.

r? @workingjubilee

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

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:

1. _11 has two initializers, so it's ignored by all SsaLocals-based transforms.
2. 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.

Ah, I see.

@bors try @rust-timer queue

⌛ Trying commit fcb91b8 with merge 5c944f0...

whoops.

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

weird.
@bors r+

📌 Commit fcb91b8 has been approved by workingjubilee

It is now in the queue for this repository.

@bors r-

pls do not r+ a try build

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

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?

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

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

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%)

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

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)

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.

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.)

📌 Commit eae5ed6 has been approved by workingjubilee

It is now in the queue for this repository.

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

@rust-timer build e87d9fc

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%)