Auto merge of #161081 - scottmcm:min-max-intrinsics, r=clarfonthey

Add intrinsics for integer minimum and maximum



I got inspired to do this when looking at `SliceOrd::compare` where I was reminded that `if a < b { a } else { b }` isn't great in MIR since it takes 4 BBs.  Looking at the codegen side, it turns out we currently emit [42 lines of LLVM-IR including 4 `alloca`s](https://rust.godbolt.org/z/ha4h4nT3r) for `u16::max` (pre-optimization), which is also unnecessarily bad†.

But both LLVM and Cranelift have dedicated things for min & max:

- https://llvm.org/docs/LangRef.html#llvm-umax-intrinsic
- https://docs.rs/cranelift-codegen/latest/cranelift_codegen/ir/trait.InstBuilder.html#method.smin

so let's just use those directly!

This actually wouldn't have been worth doing originally, but a couple of things have happened to change that:

- Back in 1.0 there was only `cmp::min` & `cmp::max`, so there was no place to actually do this at all, but in 2017 they were added to `Ord` as overridable things #25663 (comment)
- LLVM originally used icmp+select for these, not a dedicated construct, but then added one and as of 2022 the intrinsic is fully usable https://www.npopov.com/2022/12/20/This-year-in-LLVM-2022.html#integer-minmax-intrinsics
- Before we had intrinsic fallback this would have been more annoying to support everywhere -- GCC, [128-bit numbers on cg_clif](bytecodealliance/wasmtime#13790), CTFE, anything out-of-tree -- but now that we can write the obvious fallback we don't need to worry about that.
- The intrinsic would have helped less when it forced extra BBs and `alloca`s in codegen anyway, but [now](rust-lang/compiler-team#970) we can keep the result in SSA without needing to make it a primitive.

† Admittedly we could clean up the gratuitous badness there without needing an intrinsic, but I like doing the intrinsic anyway because that's the only way to avoid it always being stuck in the non-SSA path from the multi-BB assignments.  Even if we made it inlineable, GVN and such will still just give up on seeing the `x = if a < b { a } else { b }` because it's multiple assignments to the same Local, which is non-ideal for something primitive-like.

Author: bors

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

@@ -630,6 +630,26 @@ fn codegen_regular_intrinsic_call<'tcx>(
             let res = crate::num::codegen_int_binop(fx, BinOp::Div, x, y);
             ret.write_cvalue(fx, res);
         }
+        // FIXME: remove the guard here once `umin.i128` and friends are supported
+        // cc https://github.com/bytecodealliance/wasmtime/issues/13790
+        sym::integer_max | sym::integer_min if ret.layout().size <= Size::from_bits(64) => {
+            intrinsic_args!(fx, args => (lhs, rhs); intrinsic);
+
+            assert_eq!(lhs.layout().ty, rhs.layout().ty);
+            let signed = type_sign(lhs.layout().ty);
+            let lhs = lhs.load_scalar(fx);
+            let rhs = rhs.load_scalar(fx);
+            let res = match (intrinsic, signed) {
+                (sym::integer_max, false) => fx.bcx.ins().umax(lhs, rhs),
+                (sym::integer_max, true) => fx.bcx.ins().smax(lhs, rhs),
+                (sym::integer_min, false) => fx.bcx.ins().umin(lhs, rhs),
+                (sym::integer_min, true) => fx.bcx.ins().smin(lhs, rhs),
+                _ => unreachable!(),
+            };
+
+            let res = CValue::by_val(res, ret.layout());
+            ret.write_cvalue(fx, res);
+        }
         sym::saturating_add | sym::saturating_sub => {
             intrinsic_args!(fx, args => (lhs, rhs); intrinsic);
 

compiler/rustc_codegen_llvm/src/intrinsic.rs

@@ -476,6 +476,8 @@ impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
             | sym::ctpop
             | sym::bswap
             | sym::bitreverse
+            | sym::integer_max
+            | sym::integer_min
             | sym::saturating_add
             | sym::saturating_sub
             | sym::unchecked_funnel_shl
@@ -520,6 +522,18 @@ impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
                     sym::bitreverse => {
                         self.call_intrinsic("llvm.bitreverse", &[llty], &[args[0].immediate()])
                     }
+                    sym::integer_min | sym::integer_max => {
+                        let lhs = args[0].immediate();
+                        let rhs = args[1].immediate();
+                        let llvm_name = match (name, signed) {
+                            (sym::integer_max, false) => "llvm.umax",
+                            (sym::integer_max, true) => "llvm.smax",
+                            (sym::integer_min, false) => "llvm.umin",
+                            (sym::integer_min, true) => "llvm.smin",
+                            _ => bug!(),
+                        };
+                        self.call_intrinsic(llvm_name, &[llty], &[lhs, rhs])
+                    }
                     sym::unchecked_funnel_shl | sym::unchecked_funnel_shr => {
                         let is_left = name == sym::unchecked_funnel_shl;
                         let lhs = args[0].immediate();

compiler/rustc_codegen_llvm/src/lib.rs

@@ -333,6 +333,8 @@ impl CodegenBackend for LlvmCodegenBackend {
             sym::unchecked_funnel_shl,
             sym::unchecked_funnel_shr,
             sym::carrying_mul_add,
+            sym::integer_max,
+            sym::integer_min,
 
             // Fallback via libm, but the LLVM intrinsic is used instead.
             sym::sinf16, sym::sinf32, sym::sinf64,

compiler/rustc_hir_analysis/src/check/intrinsic.rs

@@ -135,6 +135,8 @@ fn intrinsic_operation_unsafety(tcx: TyCtxt<'_>, intrinsic_id: LocalDefId) -> hi
         | sym::frem_algebraic
         | sym::fsub_algebraic
         | sym::gpu_launch_sized_workgroup_mem
+        | sym::integer_max
+        | sym::integer_min
         | sym::is_val_statically_known
         | sym::log2f16
         | sym::log2f32
@@ -602,6 +604,7 @@ pub(crate) fn check_intrinsic_type(
             vec![Ty::new_imm_ptr(tcx, param(0)), Ty::new_imm_ptr(tcx, param(0))],
             tcx.types.usize,
         ),
+        sym::integer_max | sym::integer_min => (1, 0, vec![param(0), param(0)], param(0)),
         sym::unchecked_div | sym::unchecked_rem | sym::exact_div | sym::disjoint_bitor => {
             (1, 0, vec![param(0), param(0)], param(0))
         }

compiler/rustc_span/src/symbol.rs

@@ -1145,6 +1145,8 @@ symbols! {
         instruction_set,
         instrument_fn,
         integer_: "integer", // underscore to avoid clashing with the function `sym::integer` below
+        integer_max,
+        integer_min,
         integral,
         internal,
         internal_eq_trait_method_impls,

library/core/src/cmp.rs

@@ -2298,8 +2298,37 @@ mod impls {
 
     partial_ord_impl! { f16 f32 f64 f128 }
 
+    macro_rules! min_max_impl {
+        (char) => {
+            #[inline]
+            fn min(self, other: Self) -> Self {
+                let c = u32::min(self as u32, other as u32);
+                // SAFETY: it's one of the inputs
+                unsafe { char::from_u32_unchecked(c) }
+            }
+
+            #[inline]
+            fn max(self, other: Self) -> Self {
+                let c = u32::max(self as u32, other as u32);
+                // SAFETY: it's one of the inputs
+                unsafe { char::from_u32_unchecked(c) }
+            }
+        };
+        ($t:ident) => {
+            #[inline]
+            fn min(self, other: Self) -> Self {
+                crate::intrinsics::integer_min(self, other)
+            }
+
+            #[inline]
+            fn max(self, other: Self) -> Self {
+                crate::intrinsics::integer_max(self, other)
+            }
+        };
+    }
+
     macro_rules! ord_impl {
-        ($($t:ty)*) => ($(
+        ($($t:ident)*) => ($(
             #[stable(feature = "rust1", since = "1.0.0")]
             #[rustc_const_unstable(feature = "const_cmp", issue = "143800")]
             const impl PartialOrd for $t {
@@ -2338,6 +2367,8 @@ mod impls {
                         self
                     }
                 }
+
+                min_max_impl!($t);
             }
         )*)
     }

library/core/src/intrinsics/bounds.rs

@@ -109,3 +109,21 @@ const unsafe impl FloatPrimitive for f128 {
         f128::from_bits(bits)
     }
 }
+
+/// Built-in integer types (i8, i16, .., i128, isize, u8, u16, .., u128, usize).
+///
+/// Intentionally does not include other integer-repr types like `bool` or `char`.
+///
+/// # Safety
+/// Must actually *be* such a type.
+#[rustc_const_unstable(feature = "core_intrinsics", issue = "none")]
+pub const unsafe trait IntegerPrimitive: Copy + [const] Ord {}
+
+macro_rules! impl_integer_primitive {
+    ($($t:ty),*) => {$(
+        #[rustc_const_unstable(feature = "core_intrinsics", issue = "none")]
+        const unsafe impl IntegerPrimitive for $t {}
+    )*};
+}
+impl_integer_primitive!(i8, i16, i32, i64, i128, isize);
+impl_integer_primitive!(u8, u16, u32, u64, u128, usize);

library/core/src/intrinsics/mod.rs

@@ -1841,6 +1841,34 @@ pub const fn fdiv_algebraic<T: bounds::FloatPrimitive>(a: T, b: T) -> T;
 #[rustc_intrinsic]
 pub const fn frem_algebraic<T: bounds::FloatPrimitive>(a: T, b: T) -> T;
 
+/// Integer `min`imum, signed or unsigned depending on `T`.
+///
+/// Allowed only on `uN`, `iN`, `usize`, and `isize`.
+/// (Not on `bool` nor on `char`.)
+///
+/// Stabilized as [`u16::min`] and [`i64::min`] and similar.
+#[rustc_const_unstable(feature = "const_cmp", issue = "143800")]
+#[rustc_nounwind]
+#[rustc_intrinsic]
+#[miri::intrinsic_fallback_is_spec]
+pub const fn integer_min<T: [const] bounds::IntegerPrimitive>(a: T, b: T) -> T {
+    if a < b { a } else { b }
+}
+
+/// Integer `max`imum, signed or unsigned depending on `T`.
+///
+/// Allowed only on `uN`, `iN`, `usize`, and `isize`.
+/// (Not on `bool` nor on `char`.)
+///
+/// Stabilized as [`u16::max`] and [`i64::max`] and similar.
+#[rustc_const_unstable(feature = "const_cmp", issue = "143800")]
+#[rustc_nounwind]
+#[rustc_intrinsic]
+#[miri::intrinsic_fallback_is_spec]
+pub const fn integer_max<T: [const] bounds::IntegerPrimitive>(a: T, b: T) -> T {
+    if a < b { b } else { a }
+}
+
 /// Returns the number of bits set in an integer type `T`
 ///
 /// Note that, unlike most intrinsics, this is safe to call;

library/coretests/tests/cmp.rs

@@ -27,6 +27,20 @@ fn test_mut_int_totalord() {
     assert_eq!((&mut 12).cmp(&&mut -5), Greater);
 }
 
+#[test]
+fn test_max_min_signedness() {
+    use std::cmp::{max, min};
+    // Check the "same" 8-bit values where the signedness of the operation matters
+    assert_eq!(max::<u8>(0, 255), 255);
+    assert_eq!(max::<u8>(255, 0), 255);
+    assert_eq!(min::<u8>(0, 255), 0);
+    assert_eq!(min::<u8>(255, 0), 0);
+    assert_eq!(max::<i8>(0, -1), 0);
+    assert_eq!(max::<i8>(-1, 0), 0);
+    assert_eq!(min::<i8>(0, -1), -1);
+    assert_eq!(min::<i8>(-1, 0), -1);
+}
+
 #[test]
 fn test_ord_max_min() {
     assert_eq!(1.max(2), 2);

tests/codegen-llvm/intrinsics/integer_min_max.rs

@@ -0,0 +1,51 @@
+//@ compile-flags: -C opt-level=3 -C no-prepopulate-passes
+
+#![crate_type = "lib"]
+
+#[unsafe(no_mangle)]
+pub fn i16_min(a: i16, b: i16) -> i16 {
+    // CHECK-LABEL: i16_min
+    // CHECK: [[M:%.+]] = call i16 @llvm.smin.i16(i16 %a, i16 %b)
+    // CHECK-NEXT: ret i16 [[M]]
+    std::cmp::min(a, b)
+}
+
+#[unsafe(no_mangle)]
+pub fn i32_max(a: i32, b: i32) -> i32 {
+    // CHECK-LABEL: i32_max
+    // CHECK: [[M:%.+]] = call i32 @llvm.smax.i32(i32 %a, i32 %b)
+    // CHECK-NEXT: ret i32 [[M]]
+    std::cmp::max(a, b)
+}
+
+#[unsafe(no_mangle)]
+pub fn u8_min(a: u8, b: u8) -> u8 {
+    // CHECK-LABEL: u8_min
+    // CHECK: [[M:%.+]] = call i8 @llvm.umin.i8(i8 %a, i8 %b)
+    // CHECK-NEXT: ret i8 [[M]]
+    std::cmp::min(a, b)
+}
+
+#[unsafe(no_mangle)]
+pub fn u16_max(a: u16, b: u16) -> u16 {
+    // CHECK-LABEL: u16_max
+    // CHECK: [[M:%.+]] = call i16 @llvm.umax.i16(i16 %a, i16 %b)
+    // CHECK-NEXT: ret i16 [[M]]
+    std::cmp::max(a, b)
+}
+
+#[unsafe(no_mangle)]
+pub fn char_min(a: char, b: char) -> char {
+    // CHECK-LABEL: char_min
+    // CHECK: [[M:%.+]] = call i32 @llvm.umin.i32(i32 %a, i32 %b)
+    // CHECK: ret i32 [[M]]
+    std::cmp::min(a, b)
+}
+
+#[unsafe(no_mangle)]
+pub fn char_max(a: char, b: char) -> char {
+    // CHECK-LABEL: char_max
+    // CHECK: [[M:%.+]] = call i32 @llvm.umax.i32(i32 %a, i32 %b)
+    // CHECK: ret i32 [[M]]
+    std::cmp::max(a, b)
+}