Auto merge of rust-lang#118310 - scottmcm:three-way-compare, r=davidtwco

Add `Ord::cmp` for primitives as a `BinOp` in MIR

Update: most of this OP was written months ago.  See rust-lang#118310 (comment) below for where we got to recently that made it ready for review.

---

There are dozens of reasonable ways to implement `Ord::cmp` for integers using comparison, bit-ops, and branches.  Those differences are irrelevant at the rust level, however, so we can make things better by adding `BinOp::Cmp` at the MIR level:

1. Exactly how to implement it is left up to the backends, so LLVM can use whatever pattern its optimizer best recognizes and cranelift can use whichever pattern codegens the fastest.
2. By not inlining those details for every use of `cmp`, we drastically reduce the amount of MIR generated for `derive`d `PartialOrd`, while also making it more amenable to MIR-level optimizations.

Having extremely careful `if` ordering to μoptimize resource usage on broadwell (rust-lang#63767) is great, but it really feels to me like libcore is the wrong place to put that logic.  Similarly, using subtraction [tricks](https://graphics.stanford.edu/~seander/bithacks.html#CopyIntegerSign) (rust-lang#105840) is arguably even nicer, but depends on the optimizer understanding it (llvm/llvm-project#73417) to be practical.  Or maybe [bitor is better than add](https://discourse.llvm.org/t/representing-in-ir/67369/2?u=scottmcm)?  But maybe only on a future version that [has `or disjoint` support](https://discourse.llvm.org/t/rfc-add-or-disjoint-flag/75036?u=scottmcm)?  And just because one of those forms happens to be good for LLVM, there's no guarantee that it'd be the same form that GCC or Cranelift would rather see -- especially given their very different optimizers.  Not to mention that if LLVM gets a spaceship intrinsic -- [which it should](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Suboptimal.20inlining.20in.20std.20function.20.60binary_search.60/near/404250586) -- we'll need at least a rustc intrinsic to be able to call it.

As for simplifying it in Rust, we now regularly inline `{integer}::partial_cmp`, but it's quite a large amount of IR.  The best way to see that is with rust-lang@8811efa#diff-d134c32d028fbe2bf835fef2df9aca9d13332dd82284ff21ee7ebf717bfa4765R113 -- I added a new pre-codegen MIR test for a simple 3-tuple struct, and this PR change it from 36 locals and 26 basic blocks down to 24 locals and 8 basic blocks.  Even better, as soon as the construct-`Some`-then-match-it-in-same-BB noise is cleaned up, this'll expose the `Cmp == 0` branches clearly in MIR, so that an InstCombine (rust-lang#105808) can simplify that to just a `BinOp::Eq` and thus fix some of our generated code perf issues.  (Tracking that through today's `if a < b { Less } else if a == b { Equal } else { Greater }` would be *much* harder.)

---

r? `@ghost`
But first I should check that perf is ok with this
~~...and my true nemesis, tidy.~~

Author: bors

core/src/cmp.rs

@@ -376,6 +376,10 @@ pub struct AssertParamIsEq<T: Eq + ?Sized> {
 /// ```
 #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
 #[stable(feature = "rust1", since = "1.0.0")]
+// This is a lang item only so that `BinOp::Cmp` in MIR can return it.
+// It has no special behaviour, but does require that the three variants
+// `Less`/`Equal`/`Greater` remain `-1_i8`/`0_i8`/`+1_i8` respectively.
+#[cfg_attr(not(bootstrap), lang = "Ordering")]
 #[repr(i8)]
 pub enum Ordering {
     /// An ordering where a compared value is less than another.
@@ -1554,7 +1558,14 @@ mod impls {
             impl PartialOrd for $t {
                 #[inline]
                 fn partial_cmp(&self, other: &$t) -> Option<Ordering> {
-                    Some(self.cmp(other))
+                    #[cfg(bootstrap)]
+                    {
+                        Some(self.cmp(other))
+                    }
+                    #[cfg(not(bootstrap))]
+                    {
+                        Some(crate::intrinsics::three_way_compare(*self, *other))
+                    }
                 }
                 #[inline(always)]
                 fn lt(&self, other: &$t) -> bool { (*self) < (*other) }
@@ -1570,11 +1581,18 @@ mod impls {
             impl Ord for $t {
                 #[inline]
                 fn cmp(&self, other: &$t) -> Ordering {
-                    // The order here is important to generate more optimal assembly.
-                    // See <https://github.com/rust-lang/rust/issues/63758> for more info.
-                    if *self < *other { Less }
-                    else if *self == *other { Equal }
-                    else { Greater }
+                    #[cfg(bootstrap)]
+                    {
+                        // The order here is important to generate more optimal assembly.
+                        // See <https://github.com/rust-lang/rust/issues/63758> for more info.
+                        if *self < *other { Less }
+                        else if *self == *other { Equal }
+                        else { Greater }
+                    }
+                    #[cfg(not(bootstrap))]
+                    {
+                        crate::intrinsics::three_way_compare(*self, *other)
+                    }
                 }
             }
         )*)

core/src/intrinsics.rs

@@ -2146,6 +2146,18 @@ extern "rust-intrinsic" {
     #[rustc_nounwind]
     pub fn bitreverse<T: Copy>(x: T) -> T;
 
+    /// Does a three-way comparison between the two integer arguments.
+    ///
+    /// This is included as an intrinsic as it's useful to let it be one thing
+    /// in MIR, rather than the multiple checks and switches that make its IR
+    /// large and difficult to optimize.
+    ///
+    /// The stabilized version of this intrinsic is [`Ord::cmp`].
+    #[cfg(not(bootstrap))]
+    #[rustc_const_unstable(feature = "const_three_way_compare", issue = "none")]
+    #[rustc_safe_intrinsic]
+    pub fn three_way_compare<T: Copy>(lhs: T, rhs: T) -> crate::cmp::Ordering;
+
     /// Performs checked integer addition.
     ///
     /// Note that, unlike most intrinsics, this is safe to call;

core/tests/intrinsics.rs

@@ -99,3 +99,30 @@ fn test_const_deallocate_at_runtime() {
         const_deallocate(core::ptr::null_mut(), 1, 1); // nop
     }
 }
+
+#[cfg(not(bootstrap))]
+#[test]
+fn test_three_way_compare_in_const_contexts() {
+    use core::cmp::Ordering::{self, *};
+    use core::intrinsics::three_way_compare;
+
+    const UNSIGNED_LESS: Ordering = three_way_compare(123_u16, 456);
+    const UNSIGNED_EQUAL: Ordering = three_way_compare(456_u16, 456);
+    const UNSIGNED_GREATER: Ordering = three_way_compare(789_u16, 456);
+    const CHAR_LESS: Ordering = three_way_compare('A', 'B');
+    const CHAR_EQUAL: Ordering = three_way_compare('B', 'B');
+    const CHAR_GREATER: Ordering = three_way_compare('C', 'B');
+    const SIGNED_LESS: Ordering = three_way_compare(123_i64, 456);
+    const SIGNED_EQUAL: Ordering = three_way_compare(456_i64, 456);
+    const SIGNED_GREATER: Ordering = three_way_compare(789_i64, 456);
+
+    assert_eq!(UNSIGNED_LESS, Less);
+    assert_eq!(UNSIGNED_EQUAL, Equal);
+    assert_eq!(UNSIGNED_GREATER, Greater);
+    assert_eq!(CHAR_LESS, Less);
+    assert_eq!(CHAR_EQUAL, Equal);
+    assert_eq!(CHAR_GREATER, Greater);
+    assert_eq!(SIGNED_LESS, Less);
+    assert_eq!(SIGNED_EQUAL, Equal);
+    assert_eq!(SIGNED_GREATER, Greater);
+}

core/tests/lib.rs

@@ -22,6 +22,7 @@
 #![feature(const_pointer_is_aligned)]
 #![feature(const_ptr_as_ref)]
 #![feature(const_ptr_write)]
+#![cfg_attr(not(bootstrap), feature(const_three_way_compare))]
 #![feature(const_trait_impl)]
 #![feature(const_likely)]
 #![feature(const_location_fields)]