-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Micro-optimize Ord::cmp for primitives #105840
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -401,12 +401,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. | ||
|
@@ -420,12 +426,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. | ||
|
@@ -439,12 +445,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. | ||
|
@@ -458,12 +464,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. | ||
|
@@ -477,12 +483,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. | ||
|
@@ -496,12 +502,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`. | ||
|
@@ -1169,7 +1175,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> { | |
#[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 `<=` | ||
|
@@ -1186,7 +1192,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> { | |
#[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. | ||
|
@@ -1202,7 +1208,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> { | |
#[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 `>=` | ||
|
@@ -1219,7 +1225,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> { | |
#[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 } | ||
} | ||
} | ||
|
||
|
@@ -1563,12 +1569,31 @@ 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 } | ||
let mut res = 0i8; | ||
res -= (*self < *other) as i8; | ||
res += (*self > *other) as i8; | ||
// SAFETY: The discriminants of Ord were chosen to permit this | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this should say "Ordering" rather than "Ord". Should there be a comment near the discriminant values mentioning that safety of some impls depends on the values not being changed? Or a static assertion here that the discriminant values are the expected ones? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The discriminants are visible on stable and it's explicitly |
||
unsafe { crate::mem::transmute(res) } | ||
} | ||
|
||
#[inline] | ||
fn max(self, other: Self) -> Self { | ||
if self > other { | ||
self | ||
} else { | ||
other | ||
} | ||
} | ||
|
||
#[inline] | ||
fn min(self, other: Self) -> Self { | ||
if self > other { | ||
other | ||
} else { | ||
self | ||
} | ||
} | ||
|
||
} | ||
)*) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
// The `derive(PartialOrd)` for a newtype doesn't override `lt`/`le`/`gt`/`ge`. | ||
// This double-checks that the `Option<Ordering>` 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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: if this ends up still being needed, I think it'd be good to have a
FIXME
comment to move it back to being derived once that's no longer a perf regression.(Although I'm worried that if it is still needed, then everyone else's newtypes like this might also be regressed, which seems pretty scary, and a reason it might not want to land at all.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that if this is required it is reason to not land this. I'm only leaving this in for now because I want to isolate effects as much as possible.