From 6e1c3f05fd0f2f3faee05a9e541880e1bd3ed7bb Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 22 Dec 2022 13:44:51 -0800 Subject: [PATCH 1/2] Tweak the default `PartialOrd::{lt,le,gt,ge}` --- library/core/src/cmp.rs | 54 +++++++++++++------ .../codegen/newtype-relational-operators.rs | 49 +++++++++++++++++ 2 files changed, 87 insertions(+), 16 deletions(-) create mode 100644 src/test/codegen/newtype-relational-operators.rs diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index 949896e574806..7a55189bac2db 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -360,12 +360,18 @@ impl Ordering { /// assert_eq!(Ordering::Equal.is_eq(), true); /// assert_eq!(Ordering::Greater.is_eq(), false); /// ``` - #[inline] + #[inline(always)] #[must_use] #[rustc_const_stable(feature = "ordering_helpers", since = "1.53.0")] #[stable(feature = "ordering_helpers", since = "1.53.0")] pub const fn is_eq(self) -> bool { - matches!(self, Equal) + // Implementation note: It appears (as of 2022-12) that LLVM has an + // easier time with a comparison against zero like this, as opposed + // to looking for the `Less` value (-1) specifically, maybe because + // it's not always obvious to it that -2 isn't possible. + // Thus this and its siblings below are written this way, rather + // than the potentially-more-obvious `matches!` version. + (self as i8) == 0 } /// Returns `true` if the ordering is not the `Equal` variant. @@ -379,12 +385,12 @@ impl Ordering { /// assert_eq!(Ordering::Equal.is_ne(), false); /// assert_eq!(Ordering::Greater.is_ne(), true); /// ``` - #[inline] + #[inline(always)] #[must_use] #[rustc_const_stable(feature = "ordering_helpers", since = "1.53.0")] #[stable(feature = "ordering_helpers", since = "1.53.0")] pub const fn is_ne(self) -> bool { - !matches!(self, Equal) + (self as i8) != 0 } /// Returns `true` if the ordering is the `Less` variant. @@ -398,12 +404,12 @@ impl Ordering { /// assert_eq!(Ordering::Equal.is_lt(), false); /// assert_eq!(Ordering::Greater.is_lt(), false); /// ``` - #[inline] + #[inline(always)] #[must_use] #[rustc_const_stable(feature = "ordering_helpers", since = "1.53.0")] #[stable(feature = "ordering_helpers", since = "1.53.0")] pub const fn is_lt(self) -> bool { - matches!(self, Less) + (self as i8) < 0 } /// Returns `true` if the ordering is the `Greater` variant. @@ -417,12 +423,12 @@ impl Ordering { /// assert_eq!(Ordering::Equal.is_gt(), false); /// assert_eq!(Ordering::Greater.is_gt(), true); /// ``` - #[inline] + #[inline(always)] #[must_use] #[rustc_const_stable(feature = "ordering_helpers", since = "1.53.0")] #[stable(feature = "ordering_helpers", since = "1.53.0")] pub const fn is_gt(self) -> bool { - matches!(self, Greater) + (self as i8) > 0 } /// Returns `true` if the ordering is either the `Less` or `Equal` variant. @@ -436,12 +442,12 @@ impl Ordering { /// assert_eq!(Ordering::Equal.is_le(), true); /// assert_eq!(Ordering::Greater.is_le(), false); /// ``` - #[inline] + #[inline(always)] #[must_use] #[rustc_const_stable(feature = "ordering_helpers", since = "1.53.0")] #[stable(feature = "ordering_helpers", since = "1.53.0")] pub const fn is_le(self) -> bool { - !matches!(self, Greater) + (self as i8) <= 0 } /// Returns `true` if the ordering is either the `Greater` or `Equal` variant. @@ -455,12 +461,12 @@ impl Ordering { /// assert_eq!(Ordering::Equal.is_ge(), true); /// assert_eq!(Ordering::Greater.is_ge(), true); /// ``` - #[inline] + #[inline(always)] #[must_use] #[rustc_const_stable(feature = "ordering_helpers", since = "1.53.0")] #[stable(feature = "ordering_helpers", since = "1.53.0")] pub const fn is_ge(self) -> bool { - !matches!(self, Less) + (self as i8) >= 0 } /// Reverses the `Ordering`. @@ -1124,7 +1130,11 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn lt(&self, other: &Rhs) -> bool { - matches!(self.partial_cmp(other), Some(Less)) + if let Some(ordering) = self.partial_cmp(other) { + ordering.is_lt() + } else { + false + } } /// This method tests less than or equal to (for `self` and `other`) and is used by the `<=` @@ -1143,7 +1153,11 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn le(&self, other: &Rhs) -> bool { - matches!(self.partial_cmp(other), Some(Less | Equal)) + if let Some(ordering) = self.partial_cmp(other) { + ordering.is_le() + } else { + false + } } /// This method tests greater than (for `self` and `other`) and is used by the `>` operator. @@ -1161,7 +1175,11 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn gt(&self, other: &Rhs) -> bool { - matches!(self.partial_cmp(other), Some(Greater)) + if let Some(ordering) = self.partial_cmp(other) { + ordering.is_gt() + } else { + false + } } /// This method tests greater than or equal to (for `self` and `other`) and is used by the `>=` @@ -1180,7 +1198,11 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn ge(&self, other: &Rhs) -> bool { - matches!(self.partial_cmp(other), Some(Greater | Equal)) + if let Some(ordering) = self.partial_cmp(other) { + ordering.is_ge() + } else { + false + } } } diff --git a/src/test/codegen/newtype-relational-operators.rs b/src/test/codegen/newtype-relational-operators.rs new file mode 100644 index 0000000000000..5cf6c3ac0a233 --- /dev/null +++ b/src/test/codegen/newtype-relational-operators.rs @@ -0,0 +1,49 @@ +// The `derive(PartialOrd)` for a newtype doesn't override `lt`/`le`/`gt`/`ge`. +// This double-checks that the `Option` intermediate values used +// in the operators for such a type all optimize away. + +// compile-flags: -C opt-level=1 +// min-llvm-version: 15.0 + +#![crate_type = "lib"] + +use std::cmp::Ordering; + +#[derive(PartialOrd, PartialEq)] +pub struct Foo(u16); + +// CHECK-LABEL: @check_lt +// CHECK-SAME: (i16 %[[A:.+]], i16 %[[B:.+]]) +#[no_mangle] +pub fn check_lt(a: Foo, b: Foo) -> bool { + // CHECK: %[[R:.+]] = icmp ult i16 %[[A]], %[[B]] + // CHECK-NEXT: ret i1 %[[R]] + a < b +} + +// CHECK-LABEL: @check_le +// CHECK-SAME: (i16 %[[A:.+]], i16 %[[B:.+]]) +#[no_mangle] +pub fn check_le(a: Foo, b: Foo) -> bool { + // CHECK: %[[R:.+]] = icmp ule i16 %[[A]], %[[B]] + // CHECK-NEXT: ret i1 %[[R]] + a <= b +} + +// CHECK-LABEL: @check_gt +// CHECK-SAME: (i16 %[[A:.+]], i16 %[[B:.+]]) +#[no_mangle] +pub fn check_gt(a: Foo, b: Foo) -> bool { + // CHECK: %[[R:.+]] = icmp ugt i16 %[[A]], %[[B]] + // CHECK-NEXT: ret i1 %[[R]] + a > b +} + +// CHECK-LABEL: @check_ge +// CHECK-SAME: (i16 %[[A:.+]], i16 %[[B:.+]]) +#[no_mangle] +pub fn check_ge(a: Foo, b: Foo) -> bool { + // CHECK: %[[R:.+]] = icmp uge i16 %[[A]], %[[B]] + // CHECK-NEXT: ret i1 %[[R]] + a >= b +} From 118c9b5c29e09842a6e802191a6340e4ec870246 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 22 Dec 2022 14:02:42 -0800 Subject: [PATCH 2/2] Oh yeah, tidy. --- library/core/src/cmp.rs | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index 7a55189bac2db..47ea199a835c5 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -1130,11 +1130,7 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn lt(&self, other: &Rhs) -> bool { - if let Some(ordering) = self.partial_cmp(other) { - ordering.is_lt() - } else { - false - } + if let Some(ordering) = self.partial_cmp(other) { ordering.is_lt() } else { false } } /// This method tests less than or equal to (for `self` and `other`) and is used by the `<=` @@ -1153,11 +1149,7 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn le(&self, other: &Rhs) -> bool { - if let Some(ordering) = self.partial_cmp(other) { - ordering.is_le() - } else { - false - } + if let Some(ordering) = self.partial_cmp(other) { ordering.is_le() } else { false } } /// This method tests greater than (for `self` and `other`) and is used by the `>` operator. @@ -1175,11 +1167,7 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn gt(&self, other: &Rhs) -> bool { - if let Some(ordering) = self.partial_cmp(other) { - ordering.is_gt() - } else { - false - } + if let Some(ordering) = self.partial_cmp(other) { ordering.is_gt() } else { false } } /// This method tests greater than or equal to (for `self` and `other`) and is used by the `>=` @@ -1198,11 +1186,7 @@ pub trait PartialOrd: PartialEq { #[must_use] #[stable(feature = "rust1", since = "1.0.0")] fn ge(&self, other: &Rhs) -> bool { - if let Some(ordering) = self.partial_cmp(other) { - ordering.is_ge() - } else { - false - } + if let Some(ordering) = self.partial_cmp(other) { ordering.is_ge() } else { false } } }