From b54ca0e433e9cf5ef139aeebb36133be8ddee111 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 6 Mar 2025 15:09:14 -0800 Subject: [PATCH 1/3] Add a MIR pre-codegen test for tuple comparisons We have codegen ones, but it looks like we could make those less flakey by just doing something better in the first place... --- ...e_ord.demo_ge_partial.PreCodegen.after.mir | 237 ++++++++++++++++++ ...ple_ord.demo_le_total.PreCodegen.after.mir | 145 +++++++++++ tests/mir-opt/pre-codegen/tuple_ord.rs | 16 ++ 3 files changed, 398 insertions(+) create mode 100644 tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir create mode 100644 tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir create mode 100644 tests/mir-opt/pre-codegen/tuple_ord.rs diff --git a/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir b/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir new file mode 100644 index 0000000000000..696a451177791 --- /dev/null +++ b/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir @@ -0,0 +1,237 @@ +// MIR for `demo_ge_partial` after PreCodegen + +fn demo_ge_partial(_1: &(f32, f32), _2: &(f32, f32)) -> bool { + debug a => _1; + debug b => _2; + let mut _0: bool; + scope 1 (inlined std::cmp::impls::::le) { + scope 2 (inlined core::tuple::::le) { + let mut _12: bool; + let _15: std::option::Option; + let _19: &f32; + let _20: &f32; + scope 3 { + let mut _9: &std::option::Option; + let mut _13: &std::option::Option; + scope 4 (inlined as PartialEq>::ne) { + let mut _11: bool; + scope 5 (inlined as PartialEq>::eq) { + let mut _10: isize; + let mut _16: isize; + scope 6 { + scope 7 (inlined ::eq) { + let _17: i8; + scope 8 { + let _18: i8; + scope 9 { + } + } + } + } + } + } + scope 10 (inlined as PartialEq>::eq) { + let mut _14: isize; + let mut _21: isize; + scope 11 { + scope 12 (inlined ::eq) { + let _22: i8; + scope 13 { + let _23: i8; + scope 14 { + } + } + } + } + } + } + scope 15 (inlined std::cmp::impls::::partial_cmp) { + let mut _3: f32; + let mut _4: f32; + let mut _5: bool; + let mut _6: f32; + let mut _7: f32; + let mut _8: bool; + } + } + } + + bb0: { + StorageLive(_19); + StorageLive(_20); + StorageLive(_13); + StorageLive(_9); + StorageLive(_15); + StorageLive(_5); + StorageLive(_8); + StorageLive(_3); + _3 = copy ((*_1).0: f32); + StorageLive(_4); + _4 = copy ((*_2).0: f32); + _5 = Le(move _3, move _4); + StorageDead(_4); + StorageDead(_3); + StorageLive(_6); + _6 = copy ((*_1).0: f32); + StorageLive(_7); + _7 = copy ((*_2).0: f32); + _8 = Ge(move _6, move _7); + StorageDead(_7); + StorageDead(_6); + switchInt(copy _5) -> [0: bb1, otherwise: bb5]; + } + + bb1: { + switchInt(copy _8) -> [0: bb2, otherwise: bb4]; + } + + bb2: { + StorageDead(_8); + StorageDead(_5); + StorageLive(_12); + _9 = const core::tuple::::le::promoted[1]; + StorageLive(_11); + StorageLive(_16); + StorageLive(_10); + _10 = discriminant((*_9)); + _11 = Eq(copy _10, const 0_isize); + StorageDead(_10); + StorageDead(_16); + _12 = Not(move _11); + StorageDead(_11); + switchInt(move _12) -> [0: bb11, otherwise: bb3]; + } + + bb3: { + _13 = const core::tuple::::le::promoted[0]; + StorageLive(_21); + StorageLive(_14); + _14 = discriminant((*_13)); + _0 = Eq(copy _14, const 0_isize); + goto -> bb16; + } + + bb4: { + _15 = const Option::::Some(Greater); + StorageDead(_8); + StorageDead(_5); + StorageLive(_12); + _9 = const core::tuple::::le::promoted[1]; + StorageLive(_11); + StorageLive(_16); + StorageLive(_10); + goto -> bb8; + } + + bb5: { + switchInt(copy _8) -> [0: bb6, otherwise: bb7]; + } + + bb6: { + _15 = const Option::::Some(Less); + StorageDead(_8); + StorageDead(_5); + StorageLive(_12); + _9 = const core::tuple::::le::promoted[1]; + StorageLive(_11); + StorageLive(_16); + StorageLive(_10); + goto -> bb8; + } + + bb7: { + _15 = const Option::::Some(Equal); + StorageDead(_8); + StorageDead(_5); + StorageLive(_12); + _9 = const core::tuple::::le::promoted[1]; + StorageLive(_11); + StorageLive(_16); + StorageLive(_10); + goto -> bb8; + } + + bb8: { + _16 = discriminant((*_9)); + switchInt(move _16) -> [0: bb9, 1: bb10, otherwise: bb18]; + } + + bb9: { + StorageDead(_10); + StorageDead(_16); + StorageDead(_11); + _13 = const core::tuple::::le::promoted[0]; + StorageLive(_21); + StorageLive(_14); + goto -> bb13; + } + + bb10: { + StorageLive(_17); + StorageLive(_18); + _17 = discriminant(((_15 as Some).0: std::cmp::Ordering)); + _18 = discriminant((((*_9) as Some).0: std::cmp::Ordering)); + _11 = Eq(copy _17, copy _18); + StorageDead(_18); + StorageDead(_17); + StorageDead(_10); + StorageDead(_16); + _12 = Not(move _11); + StorageDead(_11); + switchInt(move _12) -> [0: bb11, otherwise: bb12]; + } + + bb11: { + _19 = &((*_1).1: f32); + _20 = &((*_2).1: f32); + _0 = ::le(move _19, move _20) -> [return: bb17, unwind continue]; + } + + bb12: { + _13 = const core::tuple::::le::promoted[0]; + StorageLive(_21); + StorageLive(_14); + goto -> bb13; + } + + bb13: { + _21 = discriminant((*_13)); + switchInt(move _21) -> [0: bb14, 1: bb15, otherwise: bb18]; + } + + bb14: { + _0 = const false; + goto -> bb16; + } + + bb15: { + StorageLive(_22); + StorageLive(_23); + _22 = discriminant(((_15 as Some).0: std::cmp::Ordering)); + _23 = discriminant((((*_13) as Some).0: std::cmp::Ordering)); + _0 = Eq(copy _22, copy _23); + StorageDead(_23); + StorageDead(_22); + goto -> bb16; + } + + bb16: { + StorageDead(_14); + StorageDead(_21); + goto -> bb17; + } + + bb17: { + StorageDead(_12); + StorageDead(_15); + StorageDead(_9); + StorageDead(_13); + StorageDead(_20); + StorageDead(_19); + return; + } + + bb18: { + unreachable; + } +} diff --git a/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir b/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir new file mode 100644 index 0000000000000..15c3ae76ae936 --- /dev/null +++ b/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir @@ -0,0 +1,145 @@ +// MIR for `demo_le_total` after PreCodegen + +fn demo_le_total(_1: &(u16, i16), _2: &(u16, i16)) -> bool { + debug a => _1; + debug b => _2; + let mut _0: bool; + scope 1 (inlined std::cmp::impls::::le) { + scope 2 (inlined core::tuple::::le) { + let mut _12: bool; + let _13: &i16; + let _14: &i16; + scope 3 { + let mut _6: &std::option::Option; + let mut _8: &std::option::Option; + scope 4 (inlined as PartialEq>::ne) { + let mut _11: bool; + scope 5 (inlined as PartialEq>::eq) { + let mut _7: isize; + scope 6 { + scope 7 (inlined ::eq) { + let _9: i8; + scope 8 { + let _10: i8; + scope 9 { + } + } + } + } + } + } + scope 10 (inlined as PartialEq>::eq) { + let mut _15: isize; + scope 11 { + scope 12 (inlined ::eq) { + let _16: i8; + scope 13 { + let _17: i8; + scope 14 { + } + } + } + } + } + } + scope 15 (inlined std::cmp::impls::::partial_cmp) { + let mut _3: u16; + let mut _4: u16; + let mut _5: std::cmp::Ordering; + } + } + } + + bb0: { + StorageLive(_13); + StorageLive(_14); + StorageLive(_8); + StorageLive(_6); + StorageLive(_3); + _3 = copy ((*_1).0: u16); + StorageLive(_4); + _4 = copy ((*_2).0: u16); + _5 = Cmp(move _3, move _4); + StorageDead(_4); + StorageDead(_3); + StorageLive(_12); + _6 = const core::tuple::::le::promoted[1]; + StorageLive(_11); + StorageLive(_7); + _7 = discriminant((*_6)); + switchInt(move _7) -> [0: bb1, 1: bb2, otherwise: bb10]; + } + + bb1: { + StorageDead(_7); + StorageDead(_11); + _8 = const core::tuple::::le::promoted[0]; + StorageLive(_15); + goto -> bb5; + } + + bb2: { + StorageLive(_9); + StorageLive(_10); + _9 = discriminant(_5); + _10 = discriminant((((*_6) as Some).0: std::cmp::Ordering)); + _11 = Eq(copy _9, copy _10); + StorageDead(_10); + StorageDead(_9); + StorageDead(_7); + _12 = Not(move _11); + StorageDead(_11); + switchInt(move _12) -> [0: bb3, otherwise: bb4]; + } + + bb3: { + _13 = &((*_1).1: i16); + _14 = &((*_2).1: i16); + _0 = ::le(move _13, move _14) -> [return: bb9, unwind continue]; + } + + bb4: { + _8 = const core::tuple::::le::promoted[0]; + StorageLive(_15); + goto -> bb5; + } + + bb5: { + _15 = discriminant((*_8)); + switchInt(move _15) -> [0: bb6, 1: bb7, otherwise: bb10]; + } + + bb6: { + _0 = const false; + goto -> bb8; + } + + bb7: { + StorageLive(_16); + StorageLive(_17); + _16 = discriminant(_5); + _17 = discriminant((((*_8) as Some).0: std::cmp::Ordering)); + _0 = Eq(copy _16, copy _17); + StorageDead(_17); + StorageDead(_16); + goto -> bb8; + } + + bb8: { + StorageDead(_15); + goto -> bb9; + } + + bb9: { + StorageDead(_12); + StorageDead(_6); + StorageDead(_8); + StorageDead(_14); + StorageDead(_13); + return; + } + + bb10: { + unreachable; + } +} diff --git a/tests/mir-opt/pre-codegen/tuple_ord.rs b/tests/mir-opt/pre-codegen/tuple_ord.rs new file mode 100644 index 0000000000000..435ac7a5be938 --- /dev/null +++ b/tests/mir-opt/pre-codegen/tuple_ord.rs @@ -0,0 +1,16 @@ +//@ compile-flags: -O -Zmir-opt-level=2 -Cdebuginfo=0 -Z inline-mir-hint-threshold=9999 +//@ needs-unwind + +#![crate_type = "lib"] + +// EMIT_MIR tuple_ord.demo_le_total.PreCodegen.after.mir +pub fn demo_le_total(a: &(u16, i16), b: &(u16, i16)) -> bool { + // CHECK-LABEL: demo_le_total + a <= b +} + +// 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 +} From 35248c6830383e67b8e2890ddd5ee48d44ad6b87 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 6 Mar 2025 15:55:40 -0800 Subject: [PATCH 2/3] Add chaining versions of lt/le/gt/ge and use them in tuple PartialOrd --- library/core/src/cmp.rs | 84 ++++++ library/core/src/tuple.rs | 25 +- ...e_ord.demo_ge_partial.PreCodegen.after.mir | 239 +++--------------- ...ple_ord.demo_le_total.PreCodegen.after.mir | 143 +++-------- tests/mir-opt/pre-codegen/tuple_ord.rs | 4 +- 5 files changed, 170 insertions(+), 325 deletions(-) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index 25bd17d5802fd..af14c4b50725f 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -29,6 +29,7 @@ mod bytewise; pub(crate) use bytewise::BytewiseEq; use self::Ordering::*; +use crate::ops::ControlFlow::{self, Break, Continue}; /// Trait for comparisons using the equality operator. /// @@ -1446,6 +1447,54 @@ pub macro PartialOrd($item:item) { /* compiler built-in */ } +/// Helpers for chaining together field PartialOrds into the full type's ordering. +/// +/// If the two values are equal, returns `ControlFlow::Continue`. +/// If the two values are not equal, returns `ControlFlow::Break(self OP other)`. +/// +/// This allows simple types like `i32` and `f64` to just emit two comparisons +/// 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. +pub(crate) trait SpecChainingPartialOrd: PartialOrd { + fn spec_chain_lt(&self, other: &Rhs) -> ControlFlow; + fn spec_chain_le(&self, other: &Rhs) -> ControlFlow; + fn spec_chain_gt(&self, other: &Rhs) -> ControlFlow; + fn spec_chain_ge(&self, other: &Rhs) -> ControlFlow; +} + +impl, U> SpecChainingPartialOrd for T { + #[inline] + default fn spec_chain_lt(&self, other: &U) -> ControlFlow { + match PartialOrd::partial_cmp(self, other) { + Some(Equal) => Continue(()), + c => Break(c.is_some_and(Ordering::is_lt)), + } + } + #[inline] + default fn spec_chain_le(&self, other: &U) -> ControlFlow { + match PartialOrd::partial_cmp(self, other) { + Some(Equal) => Continue(()), + c => Break(c.is_some_and(Ordering::is_le)), + } + } + #[inline] + default fn spec_chain_gt(&self, other: &U) -> ControlFlow { + match PartialOrd::partial_cmp(self, other) { + Some(Equal) => Continue(()), + c => Break(c.is_some_and(Ordering::is_gt)), + } + } + #[inline] + default fn spec_chain_ge(&self, other: &U) -> ControlFlow { + match PartialOrd::partial_cmp(self, other) { + Some(Equal) => Continue(()), + c => Break(c.is_some_and(Ordering::is_ge)), + } + } +} + /// Compares and returns the minimum of two values. /// /// Returns the first argument if the comparison determines them to be equal. @@ -1741,6 +1790,7 @@ where mod impls { use crate::cmp::Ordering::{self, Equal, Greater, Less}; use crate::hint::unreachable_unchecked; + use crate::ops::ControlFlow::{self, Break, Continue}; macro_rules! partial_eq_impl { ($($t:ty)*) => ($( @@ -1779,6 +1829,36 @@ mod impls { eq_impl! { () bool char usize u8 u16 u32 u64 u128 isize i8 i16 i32 i64 i128 } + macro_rules! chaining_impl { + ($t:ty) => { + // These implementations are the same for `Ord` or `PartialOrd` types + // because if either is NAN the `==` test will fail so we end up in + // the `Break` case and the comparison will correctly return `false`. + impl super::SpecChainingPartialOrd<$t> for $t { + #[inline] + fn spec_chain_lt(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs < rhs) } + } + #[inline] + fn spec_chain_le(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs <= rhs) } + } + #[inline] + fn spec_chain_gt(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs > rhs) } + } + #[inline] + fn spec_chain_ge(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs >= rhs) } + } + } + }; + } + macro_rules! partial_ord_impl { ($($t:ty)*) => ($( #[stable(feature = "rust1", since = "1.0.0")] @@ -1801,6 +1881,8 @@ mod impls { #[inline(always)] fn gt(&self, other: &$t) -> bool { (*self) > (*other) } } + + chaining_impl!($t); )*) } @@ -1840,6 +1922,8 @@ mod impls { fn gt(&self, other: &$t) -> bool { (*self) > (*other) } } + chaining_impl!($t); + #[stable(feature = "rust1", since = "1.0.0")] impl Ord for $t { #[inline] diff --git a/library/core/src/tuple.rs b/library/core/src/tuple.rs index 206b5b9e2c24f..75faaa06ee7fe 100644 --- a/library/core/src/tuple.rs +++ b/library/core/src/tuple.rs @@ -1,7 +1,9 @@ // See core/src/primitive_docs.rs for documentation. use crate::cmp::Ordering::{self, *}; +use crate::cmp::SpecChainingPartialOrd; use crate::marker::{ConstParamTy_, StructuralPartialEq, UnsizedConstParamTy}; +use crate::ops::ControlFlow::{Break, Continue}; // Recursive macro for implementing n-ary tuple functions and operations // @@ -80,19 +82,19 @@ macro_rules! tuple_impls { } #[inline] fn lt(&self, other: &($($T,)+)) -> bool { - lexical_ord!(lt, Less, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(lt, spec_chain_lt, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } #[inline] fn le(&self, other: &($($T,)+)) -> bool { - lexical_ord!(le, Less, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(le, spec_chain_le, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } #[inline] fn ge(&self, other: &($($T,)+)) -> bool { - lexical_ord!(ge, Greater, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(ge, spec_chain_ge, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } #[inline] fn gt(&self, other: &($($T,)+)) -> bool { - lexical_ord!(gt, Greater, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(gt, spec_chain_gt, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } } } @@ -171,15 +173,16 @@ macro_rules! maybe_tuple_doc { // `(a1, a2, a3) < (b1, b2, b3)` would be `lexical_ord!(lt, opt_is_lt, a1, b1, // a2, b2, a3, b3)` (and similarly for `lexical_cmp`) // -// `$ne_rel` is only used to determine the result after checking that they're -// not equal, so `lt` and `le` can both just use `Less`. +// `$chain_rel` is the method from `SpecChainingPartialOrd` to use for all but the +// final value, to produce better results for simple primitives. macro_rules! lexical_ord { - ($rel: ident, $ne_rel: ident, $a:expr, $b:expr, $($rest_a:expr, $rest_b:expr),+) => {{ - let c = PartialOrd::partial_cmp(&$a, &$b); - if c != Some(Equal) { c == Some($ne_rel) } - else { lexical_ord!($rel, $ne_rel, $($rest_a, $rest_b),+) } + ($rel: ident, $chain_rel: ident, $a:expr, $b:expr, $($rest_a:expr, $rest_b:expr),+) => {{ + match SpecChainingPartialOrd::$chain_rel(&$a, &$b) { + Break(val) => val, + Continue(()) => lexical_ord!($rel, $chain_rel, $($rest_a, $rest_b),+), + } }}; - ($rel: ident, $ne_rel: ident, $a:expr, $b:expr) => { + ($rel: ident, $chain_rel: ident, $a:expr, $b:expr) => { // Use the specific method for the last element PartialOrd::$rel(&$a, &$b) }; diff --git a/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir b/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir index 696a451177791..6531683b64450 100644 --- a/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir +++ b/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir @@ -4,234 +4,67 @@ fn demo_ge_partial(_1: &(f32, f32), _2: &(f32, f32)) -> bool { debug a => _1; debug b => _2; let mut _0: bool; - scope 1 (inlined std::cmp::impls::::le) { - scope 2 (inlined core::tuple::::le) { - let mut _12: bool; - let _15: std::option::Option; - let _19: &f32; - let _20: &f32; + scope 1 (inlined std::cmp::impls::::ge) { + scope 2 (inlined core::tuple::::ge) { + let mut _7: std::ops::ControlFlow; + let _8: bool; scope 3 { - let mut _9: &std::option::Option; - let mut _13: &std::option::Option; - scope 4 (inlined as PartialEq>::ne) { - let mut _11: bool; - scope 5 (inlined as PartialEq>::eq) { - let mut _10: isize; - let mut _16: isize; - scope 6 { - scope 7 (inlined ::eq) { - let _17: i8; - scope 8 { - let _18: i8; - scope 9 { - } - } - } - } - } - } - scope 10 (inlined as PartialEq>::eq) { - let mut _14: isize; - let mut _21: isize; - scope 11 { - scope 12 (inlined ::eq) { - let _22: i8; - scope 13 { - let _23: i8; - scope 14 { - } - } - } - } - } } - scope 15 (inlined std::cmp::impls::::partial_cmp) { + scope 4 (inlined std::cmp::impls:: for f32>::spec_chain_ge) { let mut _3: f32; let mut _4: f32; let mut _5: bool; - let mut _6: f32; - let mut _7: f32; - let mut _8: bool; + let mut _6: bool; + scope 5 { + } + } + scope 6 (inlined std::cmp::impls::::ge) { + let mut _9: f32; + let mut _10: f32; } } } bb0: { - StorageLive(_19); - StorageLive(_20); - StorageLive(_13); - StorageLive(_9); - StorageLive(_15); - StorageLive(_5); - StorageLive(_8); + StorageLive(_7); StorageLive(_3); - _3 = copy ((*_1).0: f32); StorageLive(_4); + _3 = copy ((*_1).0: f32); _4 = copy ((*_2).0: f32); - _5 = Le(move _3, move _4); - StorageDead(_4); - StorageDead(_3); - StorageLive(_6); - _6 = copy ((*_1).0: f32); - StorageLive(_7); - _7 = copy ((*_2).0: f32); - _8 = Ge(move _6, move _7); - StorageDead(_7); - StorageDead(_6); - switchInt(copy _5) -> [0: bb1, otherwise: bb5]; + StorageLive(_5); + _5 = Eq(copy _3, copy _4); + switchInt(move _5) -> [0: bb1, otherwise: bb2]; } bb1: { - switchInt(copy _8) -> [0: bb2, otherwise: bb4]; + StorageLive(_6); + _6 = Ge(copy _3, copy _4); + _7 = ControlFlow::::Break(move _6); + StorageDead(_6); + StorageDead(_5); + StorageDead(_4); + StorageDead(_3); + _8 = copy ((_7 as Break).0: bool); + _0 = copy _8; + goto -> bb3; } bb2: { - StorageDead(_8); StorageDead(_5); - StorageLive(_12); - _9 = const core::tuple::::le::promoted[1]; - StorageLive(_11); - StorageLive(_16); + StorageDead(_4); + StorageDead(_3); + StorageLive(_9); + _9 = copy ((*_1).1: f32); StorageLive(_10); - _10 = discriminant((*_9)); - _11 = Eq(copy _10, const 0_isize); + _10 = copy ((*_2).1: f32); + _0 = Ge(move _9, move _10); StorageDead(_10); - StorageDead(_16); - _12 = Not(move _11); - StorageDead(_11); - switchInt(move _12) -> [0: bb11, otherwise: bb3]; + StorageDead(_9); + goto -> bb3; } bb3: { - _13 = const core::tuple::::le::promoted[0]; - StorageLive(_21); - StorageLive(_14); - _14 = discriminant((*_13)); - _0 = Eq(copy _14, const 0_isize); - goto -> bb16; - } - - bb4: { - _15 = const Option::::Some(Greater); - StorageDead(_8); - StorageDead(_5); - StorageLive(_12); - _9 = const core::tuple::::le::promoted[1]; - StorageLive(_11); - StorageLive(_16); - StorageLive(_10); - goto -> bb8; - } - - bb5: { - switchInt(copy _8) -> [0: bb6, otherwise: bb7]; - } - - bb6: { - _15 = const Option::::Some(Less); - StorageDead(_8); - StorageDead(_5); - StorageLive(_12); - _9 = const core::tuple::::le::promoted[1]; - StorageLive(_11); - StorageLive(_16); - StorageLive(_10); - goto -> bb8; - } - - bb7: { - _15 = const Option::::Some(Equal); - StorageDead(_8); - StorageDead(_5); - StorageLive(_12); - _9 = const core::tuple::::le::promoted[1]; - StorageLive(_11); - StorageLive(_16); - StorageLive(_10); - goto -> bb8; - } - - bb8: { - _16 = discriminant((*_9)); - switchInt(move _16) -> [0: bb9, 1: bb10, otherwise: bb18]; - } - - bb9: { - StorageDead(_10); - StorageDead(_16); - StorageDead(_11); - _13 = const core::tuple::::le::promoted[0]; - StorageLive(_21); - StorageLive(_14); - goto -> bb13; - } - - bb10: { - StorageLive(_17); - StorageLive(_18); - _17 = discriminant(((_15 as Some).0: std::cmp::Ordering)); - _18 = discriminant((((*_9) as Some).0: std::cmp::Ordering)); - _11 = Eq(copy _17, copy _18); - StorageDead(_18); - StorageDead(_17); - StorageDead(_10); - StorageDead(_16); - _12 = Not(move _11); - StorageDead(_11); - switchInt(move _12) -> [0: bb11, otherwise: bb12]; - } - - bb11: { - _19 = &((*_1).1: f32); - _20 = &((*_2).1: f32); - _0 = ::le(move _19, move _20) -> [return: bb17, unwind continue]; - } - - bb12: { - _13 = const core::tuple::::le::promoted[0]; - StorageLive(_21); - StorageLive(_14); - goto -> bb13; - } - - bb13: { - _21 = discriminant((*_13)); - switchInt(move _21) -> [0: bb14, 1: bb15, otherwise: bb18]; - } - - bb14: { - _0 = const false; - goto -> bb16; - } - - bb15: { - StorageLive(_22); - StorageLive(_23); - _22 = discriminant(((_15 as Some).0: std::cmp::Ordering)); - _23 = discriminant((((*_13) as Some).0: std::cmp::Ordering)); - _0 = Eq(copy _22, copy _23); - StorageDead(_23); - StorageDead(_22); - goto -> bb16; - } - - bb16: { - StorageDead(_14); - StorageDead(_21); - goto -> bb17; - } - - bb17: { - StorageDead(_12); - StorageDead(_15); - StorageDead(_9); - StorageDead(_13); - StorageDead(_20); - StorageDead(_19); + StorageDead(_7); return; } - - bb18: { - unreachable; - } } diff --git a/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir b/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir index 15c3ae76ae936..d252052f0aef0 100644 --- a/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir +++ b/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir @@ -6,140 +6,65 @@ fn demo_le_total(_1: &(u16, i16), _2: &(u16, i16)) -> bool { let mut _0: bool; scope 1 (inlined std::cmp::impls::::le) { scope 2 (inlined core::tuple::::le) { - let mut _12: bool; - let _13: &i16; - let _14: &i16; + let mut _7: std::ops::ControlFlow; + let _8: bool; scope 3 { - let mut _6: &std::option::Option; - let mut _8: &std::option::Option; - scope 4 (inlined as PartialEq>::ne) { - let mut _11: bool; - scope 5 (inlined as PartialEq>::eq) { - let mut _7: isize; - scope 6 { - scope 7 (inlined ::eq) { - let _9: i8; - scope 8 { - let _10: i8; - scope 9 { - } - } - } - } - } - } - scope 10 (inlined as PartialEq>::eq) { - let mut _15: isize; - scope 11 { - scope 12 (inlined ::eq) { - let _16: i8; - scope 13 { - let _17: i8; - scope 14 { - } - } - } - } - } } - scope 15 (inlined std::cmp::impls::::partial_cmp) { + scope 4 (inlined std::cmp::impls:: for u16>::spec_chain_le) { let mut _3: u16; let mut _4: u16; - let mut _5: std::cmp::Ordering; + let mut _5: bool; + let mut _6: bool; + scope 5 { + } + } + scope 6 (inlined std::cmp::impls::::le) { + let mut _9: i16; + let mut _10: i16; } } } bb0: { - StorageLive(_13); - StorageLive(_14); - StorageLive(_8); - StorageLive(_6); + StorageLive(_7); StorageLive(_3); - _3 = copy ((*_1).0: u16); StorageLive(_4); + _3 = copy ((*_1).0: u16); _4 = copy ((*_2).0: u16); - _5 = Cmp(move _3, move _4); - StorageDead(_4); - StorageDead(_3); - StorageLive(_12); - _6 = const core::tuple::::le::promoted[1]; - StorageLive(_11); - StorageLive(_7); - _7 = discriminant((*_6)); - switchInt(move _7) -> [0: bb1, 1: bb2, otherwise: bb10]; + StorageLive(_5); + _5 = Eq(copy _3, copy _4); + switchInt(move _5) -> [0: bb1, otherwise: bb2]; } bb1: { - StorageDead(_7); - StorageDead(_11); - _8 = const core::tuple::::le::promoted[0]; - StorageLive(_15); - goto -> bb5; + StorageLive(_6); + _6 = Le(copy _3, copy _4); + _7 = ControlFlow::::Break(move _6); + StorageDead(_6); + StorageDead(_5); + StorageDead(_4); + StorageDead(_3); + _8 = copy ((_7 as Break).0: bool); + _0 = copy _8; + goto -> bb3; } bb2: { + StorageDead(_5); + StorageDead(_4); + StorageDead(_3); StorageLive(_9); + _9 = copy ((*_1).1: i16); StorageLive(_10); - _9 = discriminant(_5); - _10 = discriminant((((*_6) as Some).0: std::cmp::Ordering)); - _11 = Eq(copy _9, copy _10); + _10 = copy ((*_2).1: i16); + _0 = Le(move _9, move _10); StorageDead(_10); StorageDead(_9); - StorageDead(_7); - _12 = Not(move _11); - StorageDead(_11); - switchInt(move _12) -> [0: bb3, otherwise: bb4]; + goto -> bb3; } bb3: { - _13 = &((*_1).1: i16); - _14 = &((*_2).1: i16); - _0 = ::le(move _13, move _14) -> [return: bb9, unwind continue]; - } - - bb4: { - _8 = const core::tuple::::le::promoted[0]; - StorageLive(_15); - goto -> bb5; - } - - bb5: { - _15 = discriminant((*_8)); - switchInt(move _15) -> [0: bb6, 1: bb7, otherwise: bb10]; - } - - bb6: { - _0 = const false; - goto -> bb8; - } - - bb7: { - StorageLive(_16); - StorageLive(_17); - _16 = discriminant(_5); - _17 = discriminant((((*_8) as Some).0: std::cmp::Ordering)); - _0 = Eq(copy _16, copy _17); - StorageDead(_17); - StorageDead(_16); - goto -> bb8; - } - - bb8: { - StorageDead(_15); - goto -> bb9; - } - - bb9: { - StorageDead(_12); - StorageDead(_6); - StorageDead(_8); - StorageDead(_14); - StorageDead(_13); + StorageDead(_7); return; } - - bb10: { - unreachable; - } } diff --git a/tests/mir-opt/pre-codegen/tuple_ord.rs b/tests/mir-opt/pre-codegen/tuple_ord.rs index 435ac7a5be938..74a919e54246c 100644 --- a/tests/mir-opt/pre-codegen/tuple_ord.rs +++ b/tests/mir-opt/pre-codegen/tuple_ord.rs @@ -1,4 +1,4 @@ -//@ compile-flags: -O -Zmir-opt-level=2 -Cdebuginfo=0 -Z inline-mir-hint-threshold=9999 +//@ compile-flags: -O -Zmir-opt-level=2 -Cdebuginfo=0 //@ needs-unwind #![crate_type = "lib"] @@ -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 } From 7781346243c1e1a038e0bc6fa11e5e1aefea7d4a Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 23 Mar 2025 15:27:31 -0700 Subject: [PATCH 3/3] Stop using specialization for this Uses `__`-named `doc(hidden)` methods instead. --- library/core/src/cmp.rs | 162 ++++++++++-------- library/core/src/tuple.rs | 13 +- ...e_ord.demo_ge_partial.PreCodegen.after.mir | 2 +- ...ple_ord.demo_le_total.PreCodegen.after.mir | 2 +- 4 files changed, 95 insertions(+), 84 deletions(-) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index af14c4b50725f..0b0dbf723b658 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -29,7 +29,7 @@ mod bytewise; pub(crate) use bytewise::BytewiseEq; use self::Ordering::*; -use crate::ops::ControlFlow::{self, Break, Continue}; +use crate::ops::ControlFlow; /// Trait for comparisons using the equality operator. /// @@ -1436,65 +1436,78 @@ pub trait PartialOrd: PartialEq { fn ge(&self, other: &Rhs) -> bool { self.partial_cmp(other).is_some_and(Ordering::is_ge) } -} - -/// Derive macro generating an impl of the trait [`PartialOrd`]. -/// The behavior of this macro is described in detail [here](PartialOrd#derivable). -#[rustc_builtin_macro] -#[stable(feature = "builtin_macro_prelude", since = "1.38.0")] -#[allow_internal_unstable(core_intrinsics)] -pub macro PartialOrd($item:item) { - /* compiler built-in */ -} - -/// Helpers for chaining together field PartialOrds into the full type's ordering. -/// -/// If the two values are equal, returns `ControlFlow::Continue`. -/// If the two values are not equal, returns `ControlFlow::Break(self OP other)`. -/// -/// This allows simple types like `i32` and `f64` to just emit two comparisons -/// 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. -pub(crate) trait SpecChainingPartialOrd: PartialOrd { - fn spec_chain_lt(&self, other: &Rhs) -> ControlFlow; - fn spec_chain_le(&self, other: &Rhs) -> ControlFlow; - fn spec_chain_gt(&self, other: &Rhs) -> ControlFlow; - fn spec_chain_ge(&self, other: &Rhs) -> ControlFlow; -} -impl, U> SpecChainingPartialOrd for T { + /// If `self == other`, returns `ControlFlow::Continue(())`. + /// Otherwise, returns `ControlFlow::Break(self < other)`. + /// + /// This is useful for chaining together calls when implementing a lexical + /// `PartialOrd::lt`, as it allows types (like primitives) which can cheaply + /// check `==` and `<` separately to do rather than needing to calculate + /// (then optimize out) the three-way `Ordering` result. #[inline] - default fn spec_chain_lt(&self, other: &U) -> ControlFlow { - match PartialOrd::partial_cmp(self, other) { - Some(Equal) => Continue(()), - c => Break(c.is_some_and(Ordering::is_lt)), - } + #[must_use] + // Added to improve the behaviour of tuples; not necessarily stabilization-track. + #[unstable(feature = "partial_ord_chaining_methods", issue = "none")] + #[doc(hidden)] + fn __chaining_lt(&self, other: &Rhs) -> ControlFlow { + default_chaining_impl(self, other, Ordering::is_lt) } + + /// Same as `__chaining_lt`, but for `<=` instead of `<`. #[inline] - default fn spec_chain_le(&self, other: &U) -> ControlFlow { - match PartialOrd::partial_cmp(self, other) { - Some(Equal) => Continue(()), - c => Break(c.is_some_and(Ordering::is_le)), - } + #[must_use] + #[unstable(feature = "partial_ord_chaining_methods", issue = "none")] + #[doc(hidden)] + fn __chaining_le(&self, other: &Rhs) -> ControlFlow { + default_chaining_impl(self, other, Ordering::is_le) } + + /// Same as `__chaining_lt`, but for `>` instead of `<`. #[inline] - default fn spec_chain_gt(&self, other: &U) -> ControlFlow { - match PartialOrd::partial_cmp(self, other) { - Some(Equal) => Continue(()), - c => Break(c.is_some_and(Ordering::is_gt)), - } + #[must_use] + #[unstable(feature = "partial_ord_chaining_methods", issue = "none")] + #[doc(hidden)] + fn __chaining_gt(&self, other: &Rhs) -> ControlFlow { + default_chaining_impl(self, other, Ordering::is_gt) } + + /// Same as `__chaining_lt`, but for `>=` instead of `<`. #[inline] - default fn spec_chain_ge(&self, other: &U) -> ControlFlow { - match PartialOrd::partial_cmp(self, other) { - Some(Equal) => Continue(()), - c => Break(c.is_some_and(Ordering::is_ge)), - } + #[must_use] + #[unstable(feature = "partial_ord_chaining_methods", issue = "none")] + #[doc(hidden)] + fn __chaining_ge(&self, other: &Rhs) -> ControlFlow { + default_chaining_impl(self, other, Ordering::is_ge) } } +fn default_chaining_impl( + lhs: &T, + rhs: &U, + p: impl FnOnce(Ordering) -> bool, +) -> ControlFlow +where + T: PartialOrd, +{ + // It's important that this only call `partial_cmp` once, not call `eq` then + // one of the relational operators. We don't want to `bcmp`-then-`memcp` a + // `String`, for example, or similarly for other data structures (#108157). + match >::partial_cmp(lhs, rhs) { + Some(Equal) => ControlFlow::Continue(()), + Some(c) => ControlFlow::Break(p(c)), + None => ControlFlow::Break(false), + } +} + +/// Derive macro generating an impl of the trait [`PartialOrd`]. +/// The behavior of this macro is described in detail [here](PartialOrd#derivable). +#[rustc_builtin_macro] +#[stable(feature = "builtin_macro_prelude", since = "1.38.0")] +#[allow_internal_unstable(core_intrinsics)] +pub macro PartialOrd($item:item) { + /* compiler built-in */ +} + /// Compares and returns the minimum of two values. /// /// Returns the first argument if the comparison determines them to be equal. @@ -1829,32 +1842,31 @@ mod impls { eq_impl! { () bool char usize u8 u16 u32 u64 u128 isize i8 i16 i32 i64 i128 } - macro_rules! chaining_impl { + macro_rules! chaining_methods_impl { ($t:ty) => { // These implementations are the same for `Ord` or `PartialOrd` types // because if either is NAN the `==` test will fail so we end up in // the `Break` case and the comparison will correctly return `false`. - impl super::SpecChainingPartialOrd<$t> for $t { - #[inline] - fn spec_chain_lt(&self, other: &Self) -> ControlFlow { - let (lhs, rhs) = (*self, *other); - if lhs == rhs { Continue(()) } else { Break(lhs < rhs) } - } - #[inline] - fn spec_chain_le(&self, other: &Self) -> ControlFlow { - let (lhs, rhs) = (*self, *other); - if lhs == rhs { Continue(()) } else { Break(lhs <= rhs) } - } - #[inline] - fn spec_chain_gt(&self, other: &Self) -> ControlFlow { - let (lhs, rhs) = (*self, *other); - if lhs == rhs { Continue(()) } else { Break(lhs > rhs) } - } - #[inline] - fn spec_chain_ge(&self, other: &Self) -> ControlFlow { - let (lhs, rhs) = (*self, *other); - if lhs == rhs { Continue(()) } else { Break(lhs >= rhs) } - } + + #[inline] + fn __chaining_lt(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs < rhs) } + } + #[inline] + fn __chaining_le(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs <= rhs) } + } + #[inline] + fn __chaining_gt(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs > rhs) } + } + #[inline] + fn __chaining_ge(&self, other: &Self) -> ControlFlow { + let (lhs, rhs) = (*self, *other); + if lhs == rhs { Continue(()) } else { Break(lhs >= rhs) } } }; } @@ -1880,9 +1892,9 @@ mod impls { fn ge(&self, other: &$t) -> bool { (*self) >= (*other) } #[inline(always)] fn gt(&self, other: &$t) -> bool { (*self) > (*other) } - } - chaining_impl!($t); + chaining_methods_impl!($t); + } )*) } @@ -1920,9 +1932,9 @@ mod impls { fn ge(&self, other: &$t) -> bool { (*self) >= (*other) } #[inline(always)] fn gt(&self, other: &$t) -> bool { (*self) > (*other) } - } - chaining_impl!($t); + chaining_methods_impl!($t); + } #[stable(feature = "rust1", since = "1.0.0")] impl Ord for $t { diff --git a/library/core/src/tuple.rs b/library/core/src/tuple.rs index 75faaa06ee7fe..d754bb9034300 100644 --- a/library/core/src/tuple.rs +++ b/library/core/src/tuple.rs @@ -1,7 +1,6 @@ // See core/src/primitive_docs.rs for documentation. use crate::cmp::Ordering::{self, *}; -use crate::cmp::SpecChainingPartialOrd; use crate::marker::{ConstParamTy_, StructuralPartialEq, UnsizedConstParamTy}; use crate::ops::ControlFlow::{Break, Continue}; @@ -82,19 +81,19 @@ macro_rules! tuple_impls { } #[inline] fn lt(&self, other: &($($T,)+)) -> bool { - lexical_ord!(lt, spec_chain_lt, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(lt, __chaining_lt, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } #[inline] fn le(&self, other: &($($T,)+)) -> bool { - lexical_ord!(le, spec_chain_le, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(le, __chaining_le, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } #[inline] fn ge(&self, other: &($($T,)+)) -> bool { - lexical_ord!(ge, spec_chain_ge, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(ge, __chaining_ge, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } #[inline] fn gt(&self, other: &($($T,)+)) -> bool { - lexical_ord!(gt, spec_chain_gt, $( ${ignore($T)} self.${index()}, other.${index()} ),+) + lexical_ord!(gt, __chaining_gt, $( ${ignore($T)} self.${index()}, other.${index()} ),+) } } } @@ -173,11 +172,11 @@ macro_rules! maybe_tuple_doc { // `(a1, a2, a3) < (b1, b2, b3)` would be `lexical_ord!(lt, opt_is_lt, a1, b1, // a2, b2, a3, b3)` (and similarly for `lexical_cmp`) // -// `$chain_rel` is the method from `SpecChainingPartialOrd` to use for all but the +// `$chain_rel` is the chaining method from `PartialOrd` to use for all but the // final value, to produce better results for simple primitives. macro_rules! lexical_ord { ($rel: ident, $chain_rel: ident, $a:expr, $b:expr, $($rest_a:expr, $rest_b:expr),+) => {{ - match SpecChainingPartialOrd::$chain_rel(&$a, &$b) { + match PartialOrd::$chain_rel(&$a, &$b) { Break(val) => val, Continue(()) => lexical_ord!($rel, $chain_rel, $($rest_a, $rest_b),+), } diff --git a/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir b/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir index 6531683b64450..dd2eebc8f4a14 100644 --- a/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir +++ b/tests/mir-opt/pre-codegen/tuple_ord.demo_ge_partial.PreCodegen.after.mir @@ -10,7 +10,7 @@ fn demo_ge_partial(_1: &(f32, f32), _2: &(f32, f32)) -> bool { let _8: bool; scope 3 { } - scope 4 (inlined std::cmp::impls:: for f32>::spec_chain_ge) { + scope 4 (inlined std::cmp::impls::::__chaining_ge) { let mut _3: f32; let mut _4: f32; let mut _5: bool; diff --git a/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir b/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir index d252052f0aef0..ea1d164cefafb 100644 --- a/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir +++ b/tests/mir-opt/pre-codegen/tuple_ord.demo_le_total.PreCodegen.after.mir @@ -10,7 +10,7 @@ fn demo_le_total(_1: &(u16, i16), _2: &(u16, i16)) -> bool { let _8: bool; scope 3 { } - scope 4 (inlined std::cmp::impls:: for u16>::spec_chain_le) { + scope 4 (inlined std::cmp::impls::::__chaining_le) { let mut _3: u16; let mut _4: u16; let mut _5: bool;