Skip to content
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

Improve the generic MIR in the default PartialOrd::le and friends #137904

Merged
merged 2 commits into from
Mar 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions library/core/src/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,12 @@ pub enum Ordering {
}

impl Ordering {
#[inline]
const fn as_raw(self) -> i8 {
// FIXME(const-hack): just use `PartialOrd` against `Equal` once that's const
crate::intrinsics::discriminant_value(&self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious... how does this differ from an as cast? (ie. self as i8)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using as here would stick two assumes in the MIR for no good reason.

I need to go remove those now that LLVM has range attributes, don't I...

}

/// Returns `true` if the ordering is the `Equal` variant.
///
/// # Examples
Expand All @@ -413,7 +419,11 @@ impl Ordering {
#[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)
// All the `is_*` methods are implemented as comparisons against zero
// to follow how clang's libcxx implements their equivalents in
// <https://github.com/llvm/llvm-project/blob/60486292b79885b7800b082754153202bef5b1f0/libcxx/include/__compare/is_eq.h#L23-L28>

self.as_raw() == 0
}

/// Returns `true` if the ordering is not the `Equal` variant.
Expand All @@ -432,7 +442,7 @@ impl Ordering {
#[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_raw() != 0
}

/// Returns `true` if the ordering is the `Less` variant.
Expand All @@ -451,7 +461,7 @@ impl Ordering {
#[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_raw() < 0
}

/// Returns `true` if the ordering is the `Greater` variant.
Expand All @@ -470,7 +480,7 @@ impl Ordering {
#[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_raw() > 0
}

/// Returns `true` if the ordering is either the `Less` or `Equal` variant.
Expand All @@ -489,7 +499,7 @@ impl Ordering {
#[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_raw() <= 0
}

/// Returns `true` if the ordering is either the `Greater` or `Equal` variant.
Expand All @@ -508,7 +518,7 @@ impl Ordering {
#[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_raw() >= 0
}

/// Reverses the `Ordering`.
Expand Down
89 changes: 89 additions & 0 deletions tests/mir-opt/pre-codegen/derived_ord.demo_le.PreCodegen.after.mir
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// MIR for `demo_le` after PreCodegen

fn demo_le(_1: &MultiField, _2: &MultiField) -> bool {
debug a => _1;
debug b => _2;
let mut _0: bool;
scope 1 (inlined <MultiField as PartialOrd>::le) {
let mut _11: std::option::Option<std::cmp::Ordering>;
scope 2 (inlined Option::<std::cmp::Ordering>::is_some_and::<fn(std::cmp::Ordering) -> bool {std::cmp::Ordering::is_le}>) {
let _12: std::cmp::Ordering;
scope 3 {
scope 4 (inlined <fn(std::cmp::Ordering) -> bool {std::cmp::Ordering::is_le} as FnOnce<(std::cmp::Ordering,)>>::call_once - shim(fn(std::cmp::Ordering) -> bool {std::cmp::Ordering::is_le})) {
scope 5 (inlined std::cmp::Ordering::is_le) {
let mut _13: i8;
scope 6 (inlined std::cmp::Ordering::as_raw) {
}
}
}
}
}
scope 7 (inlined <MultiField as PartialOrd>::partial_cmp) {
let mut _6: std::option::Option<std::cmp::Ordering>;
let mut _7: i8;
scope 8 {
}
scope 9 (inlined std::cmp::impls::<impl PartialOrd for char>::partial_cmp) {
let mut _3: char;
let mut _4: char;
let mut _5: std::cmp::Ordering;
}
scope 10 (inlined std::cmp::impls::<impl PartialOrd for i16>::partial_cmp) {
let mut _8: i16;
let mut _9: i16;
let mut _10: std::cmp::Ordering;
}
}
}

bb0: {
StorageLive(_12);
StorageLive(_11);
StorageLive(_5);
StorageLive(_7);
StorageLive(_3);
_3 = copy ((*_1).0: char);
StorageLive(_4);
_4 = copy ((*_2).0: char);
_5 = Cmp(move _3, move _4);
StorageDead(_4);
StorageDead(_3);
_6 = Option::<std::cmp::Ordering>::Some(copy _5);
_7 = discriminant(_5);
switchInt(move _7) -> [0: bb1, otherwise: bb2];
}

bb1: {
StorageLive(_10);
StorageLive(_8);
_8 = copy ((*_1).1: i16);
StorageLive(_9);
_9 = copy ((*_2).1: i16);
_10 = Cmp(move _8, move _9);
StorageDead(_9);
StorageDead(_8);
_11 = Option::<std::cmp::Ordering>::Some(move _10);
StorageDead(_10);
StorageDead(_7);
StorageDead(_5);
goto -> bb3;
}

bb2: {
_11 = copy _6;
StorageDead(_7);
StorageDead(_5);
goto -> bb3;
}

bb3: {
_12 = move ((_11 as Some).0: std::cmp::Ordering);
StorageLive(_13);
_13 = discriminant(_12);
_0 = Le(move _13, const 0_i8);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this implementation for is_le means that we get an Le in the MIR (and similarly is_gt gives a Gt, etc)

The previous matches! implementation meant that this was

        _14 = Eq(copy _13, const 1_i8);
        _0 = Not(move _14);

which is much less obviously a <=.

(That should also make these look like what you'd get in C++ for things like (a <=> b) <= 0, https://en.cppreference.com/w/cpp/utility/compare/strong_ordering#Comparisons, and thus make LLVM more likely to continue getting them right since it's a code pattern that clang uses too:
https://github.com/llvm/llvm-project/blob/60486292b79885b7800b082754153202bef5b1f0/libcxx/include/__compare/is_eq.h#L26 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Do we have a MIR transform that would normalize that...?

Copy link
Member Author

@scottmcm scottmcm Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because we don't have an InstCombine system.

GVN can't do it for two reasons:

  1. _11 has two initializers, so it's ignored by all SsaLocals-based transforms.
  2. There's no _n = Le(…, …) in the program, and GVN can only replace things with locals or constants, not new never-seen-before Rvalues.

EDIT: err, I replied about making a Le, but you probably meant why we can't it can't be _0 = Ne(copy _13, const 1_i8). So only the latter reason applies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see.

StorageDead(_13);
StorageDead(_11);
StorageDead(_12);
return;
}
}
26 changes: 25 additions & 1 deletion tests/mir-opt/pre-codegen/derived_ord.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,33 @@
// skip-filecheck
//@ compile-flags: -O -Zmir-opt-level=2 -Cdebuginfo=0

#![crate_type = "lib"]

#[derive(PartialOrd, PartialEq)]
pub struct MultiField(char, i16);

// Because this isn't derived by the impl, it's not on the `{impl#0}-partial_cmp`,
// and thus we need to call it to see what the inlined generic one produces.
pub fn demo_le(a: &MultiField, b: &MultiField) -> bool {
// CHECK-LABEL: fn demo_le
// CHECK: inlined <MultiField as PartialOrd>::le
// CHECK: inlined{{.+}}is_some_and
// CHECK: inlined <MultiField as PartialOrd>::partial_cmp

// CHECK: [[A0:_[0-9]+]] = copy ((*_1).0: char);
// CHECK: [[B0:_[0-9]+]] = copy ((*_2).0: char);
// CHECK: Cmp(move [[A0]], move [[B0]]);

// CHECK: [[D0:_[0-9]+]] = discriminant({{.+}});
// CHECK: switchInt(move [[D0]]) -> [0: bb{{[0-9]+}}, otherwise: bb{{[0-9]+}}];

// CHECK: [[A1:_[0-9]+]] = copy ((*_1).1: i16);
// CHECK: [[B1:_[0-9]+]] = copy ((*_2).1: i16);
// CHECK: Cmp(move [[A1]], move [[B1]]);

// CHECK: [[D1:_[0-9]+]] = discriminant({{.+}});
// CHECK: _0 = Le(move [[D1]], const 0_i8);
*a <= *b
}

// EMIT_MIR derived_ord.{impl#0}-partial_cmp.PreCodegen.after.mir
// EMIT_MIR derived_ord.demo_le.PreCodegen.after.mir
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// MIR for `<impl at $DIR/derived_ord.rs:6:10: 6:20>::partial_cmp` after PreCodegen
// MIR for `<impl at $DIR/derived_ord.rs:5:10: 5:20>::partial_cmp` after PreCodegen

fn <impl at $DIR/derived_ord.rs:6:10: 6:20>::partial_cmp(_1: &MultiField, _2: &MultiField) -> Option<std::cmp::Ordering> {
fn <impl at $DIR/derived_ord.rs:5:10: 5:20>::partial_cmp(_1: &MultiField, _2: &MultiField) -> Option<std::cmp::Ordering> {
debug self => _1;
debug other => _2;
let mut _0: std::option::Option<std::cmp::Ordering>;
Expand Down
Loading