Skip to content

Commit 816f958

Browse files
borsbors
committed
Auto merge of #108157 - scottmcm:tuple-gt-via-partialcmp, r=dtolnay
Use `partial_cmp` to implement tuple `lt`/`le`/`ge`/`gt` In today's implementation, `(A, B)::gt` contains calls to *both* `A::eq` *and* `A::gt`. That's fine for primitives, but for things like `String`s it's kinda weird -- `(String, usize)::gt` has a call to both `bcmp` and `memcmp` (<https://rust.godbolt.org/z/7jbbPMesf>) because when `bcmp` says the `String`s aren't equal, it turns around and calls `memcmp` to find out which one's bigger. This PR changes the implementation to instead implement `(A, …, C, Z)::gt` using `A::partial_cmp`, `…::partial_cmp`, `C::partial_cmp`, and `Z::gt`. (And analogously for `lt`, `le`, and `ge`.) That way expensive comparisons don't need to be repeated. Technically this is an observable change on stable, so I've marked it `needs-fcp` + `T-libs-api` and will r? rust-lang/libs-api I'm hoping that this will be non-controversial, however, since it's very similar to the observable changes that were made to the derives (#81384 #98655) -- like those, this only changes behaviour if a type overrode behaviour in a way inconsistent with the rules for the various traits involved. (The first commit here is #108156, adding the codegen test, which I used to make sure this doesn't regress behaviour for primitives.) Zulip conversation about this change: <https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60.3E.60.20on.20Tuples/near/328392927>.
2 parents 7820b62 + 4492793 commit 816f958

File tree

4 files changed

+179
-13
lines changed

4 files changed

+179
-13
lines changed

‎library/core/benches/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ mod ops;
2020
mod pattern;
2121
mod slice;
2222
mod str;
23+
mod tuple;
2324

2425
/// Returns a `rand::Rng` seeded with a consistent seed.
2526
///

‎library/core/benches/tuple.rs

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
use rand::prelude::*;
2+
use test::{black_box, Bencher};
3+
4+
#[bench]
5+
fn bench_tuple_comparison(b: &mut Bencher) {
6+
let mut rng = black_box(super::bench_rng());
7+
8+
let data = black_box([
9+
("core::iter::adapters::Chain", 123_usize),
10+
("core::iter::adapters::Clone", 456_usize),
11+
("core::iter::adapters::Copie", 789_usize),
12+
("core::iter::adapters::Cycle", 123_usize),
13+
("core::iter::adapters::Flatt", 456_usize),
14+
("core::iter::adapters::TakeN", 789_usize),
15+
]);
16+
17+
b.iter(|| {
18+
let x = data.choose(&mut rng).unwrap();
19+
let y = data.choose(&mut rng).unwrap();
20+
[x < y, x <= y, x > y, x >= y]
21+
});
22+
}

‎library/core/src/tuple.rs

+35-13
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// See src/libstd/primitive_docs.rs for documentation.
22

3-
use crate::cmp::Ordering::*;
4-
use crate::cmp::*;
3+
use crate::cmp::Ordering::{self, *};
4+
use crate::mem::transmute;
55

66
// Recursive macro for implementing n-ary tuple functions and operations
77
//
@@ -61,19 +61,19 @@ macro_rules! tuple_impls {
6161
}
6262
#[inline]
6363
fn lt(&self, other: &($($T,)+)) -> bool {
64-
lexical_ord!(lt, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
64+
lexical_ord!(lt, Less, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
6565
}
6666
#[inline]
6767
fn le(&self, other: &($($T,)+)) -> bool {
68-
lexical_ord!(le, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
68+
lexical_ord!(le, Less, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
6969
}
7070
#[inline]
7171
fn ge(&self, other: &($($T,)+)) -> bool {
72-
lexical_ord!(ge, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
72+
lexical_ord!(ge, Greater, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
7373
}
7474
#[inline]
7575
fn gt(&self, other: &($($T,)+)) -> bool {
76-
lexical_ord!(gt, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
76+
lexical_ord!(gt, Greater, $( ${ignore(T)} self.${index()}, other.${index()} ),+)
7777
}
7878
}
7979
}
@@ -123,16 +123,38 @@ macro_rules! maybe_tuple_doc {
123123
};
124124
}
125125

126-
// Constructs an expression that performs a lexical ordering using method $rel.
126+
#[inline]
127+
const fn ordering_is_some(c: Option<Ordering>, x: Ordering) -> bool {
128+
// FIXME: Just use `==` once that's const-stable on `Option`s.
129+
// This isn't using `match` because that optimizes worse due to
130+
// making a two-step check (`Some` *then* the inner value).
131+
132+
// SAFETY: There's no public guarantee for `Option<Ordering>`,
133+
// but we're core so we know that it's definitely a byte.
134+
unsafe {
135+
let c: i8 = transmute(c);
136+
let x: i8 = transmute(Some(x));
137+
c == x
138+
}
139+
}
140+
141+
// Constructs an expression that performs a lexical ordering using method `$rel`.
127142
// The values are interleaved, so the macro invocation for
128-
// `(a1, a2, a3) < (b1, b2, b3)` would be `lexical_ord!(lt, a1, b1, a2, b2,
129-
// a3, b3)` (and similarly for `lexical_cmp`)
143+
// `(a1, a2, a3) < (b1, b2, b3)` would be `lexical_ord!(lt, opt_is_lt, a1, b1,
144+
// a2, b2, a3, b3)` (and similarly for `lexical_cmp`)
145+
//
146+
// `$ne_rel` is only used to determine the result after checking that they're
147+
// not equal, so `lt` and `le` can both just use `Less`.
130148
macro_rules! lexical_ord {
131-
($rel: ident, $a:expr, $b:expr, $($rest_a:expr, $rest_b:expr),+) => {
132-
if $a != $b { lexical_ord!($rel, $a, $b) }
133-
else { lexical_ord!($rel, $($rest_a, $rest_b),+) }
149+
($rel: ident, $ne_rel: ident, $a:expr, $b:expr, $($rest_a:expr, $rest_b:expr),+) => {{
150+
let c = PartialOrd::partial_cmp(&$a, &$b);
151+
if !ordering_is_some(c, Equal) { ordering_is_some(c, $ne_rel) }
152+
else { lexical_ord!($rel, $ne_rel, $($rest_a, $rest_b),+) }
153+
}};
154+
($rel: ident, $ne_rel: ident, $a:expr, $b:expr) => {
155+
// Use the specific method for the last element
156+
PartialOrd::$rel(&$a, &$b)
134157
};
135-
($rel: ident, $a:expr, $b:expr) => { ($a) . $rel (& $b) };
136158
}
137159

138160
macro_rules! lexical_partial_cmp {

‎tests/codegen/comparison-operators-2-tuple.rs

+121
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// compile-flags: -C opt-level=1 -Z merge-functions=disabled
2+
// min-llvm-version: 15.0
3+
// only-x86_64
4+
5+
#![crate_type = "lib"]
6+
7+
use std::cmp::Ordering;
8+
9+
type TwoTuple = (i16, u16);
10+
11+
//
12+
// The operators are all overridden directly, so should optimize easily.
13+
//
14+
// Yes, the `s[lg]t` is correct for the `[lg]e` version because it's only used
15+
// in the side of the select where we know the values are *not* equal.
16+
//
17+
18+
// CHECK-LABEL: @check_lt_direct
19+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
20+
#[no_mangle]
21+
pub fn check_lt_direct(a: TwoTuple, b: TwoTuple) -> bool {
22+
// CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
23+
// CHECK-DAG: %[[CMP0:.+]] = icmp slt i16 %[[A0]], %[[B0]]
24+
// CHECK-DAG: %[[CMP1:.+]] = icmp ult i16 %[[A1]], %[[B1]]
25+
// CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
26+
// CHECK: ret i1 %[[R]]
27+
a < b
28+
}
29+
30+
// CHECK-LABEL: @check_le_direct
31+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
32+
#[no_mangle]
33+
pub fn check_le_direct(a: TwoTuple, b: TwoTuple) -> bool {
34+
// CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
35+
// CHECK-DAG: %[[CMP0:.+]] = icmp slt i16 %[[A0]], %[[B0]]
36+
// CHECK-DAG: %[[CMP1:.+]] = icmp ule i16 %[[A1]], %[[B1]]
37+
// CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
38+
// CHECK: ret i1 %[[R]]
39+
a <= b
40+
}
41+
42+
// CHECK-LABEL: @check_gt_direct
43+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
44+
#[no_mangle]
45+
pub fn check_gt_direct(a: TwoTuple, b: TwoTuple) -> bool {
46+
// CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
47+
// CHECK-DAG: %[[CMP0:.+]] = icmp sgt i16 %[[A0]], %[[B0]]
48+
// CHECK-DAG: %[[CMP1:.+]] = icmp ugt i16 %[[A1]], %[[B1]]
49+
// CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
50+
// CHECK: ret i1 %[[R]]
51+
a > b
52+
}
53+
54+
// CHECK-LABEL: @check_ge_direct
55+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
56+
#[no_mangle]
57+
pub fn check_ge_direct(a: TwoTuple, b: TwoTuple) -> bool {
58+
// CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
59+
// CHECK-DAG: %[[CMP0:.+]] = icmp sgt i16 %[[A0]], %[[B0]]
60+
// CHECK-DAG: %[[CMP1:.+]] = icmp uge i16 %[[A1]], %[[B1]]
61+
// CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
62+
// CHECK: ret i1 %[[R]]
63+
a >= b
64+
}
65+
66+
//
67+
// These ones are harder, since there are more intermediate values to remove.
68+
//
69+
// `<` seems to be getting lucky right now, so test that doesn't regress.
70+
//
71+
// The others, however, aren't managing to optimize away the extra `select`s yet.
72+
// See <https://github.com/rust-lang/rust/issues/106107> for more about this.
73+
//
74+
75+
// CHECK-LABEL: @check_lt_via_cmp
76+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
77+
#[no_mangle]
78+
pub fn check_lt_via_cmp(a: TwoTuple, b: TwoTuple) -> bool {
79+
// CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
80+
// CHECK-DAG: %[[CMP0:.+]] = icmp slt i16 %[[A0]], %[[B0]]
81+
// CHECK-DAG: %[[CMP1:.+]] = icmp ult i16 %[[A1]], %[[B1]]
82+
// CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
83+
// CHECK: ret i1 %[[R]]
84+
Ord::cmp(&a, &b).is_lt()
85+
}
86+
87+
// CHECK-LABEL: @check_le_via_cmp
88+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
89+
#[no_mangle]
90+
pub fn check_le_via_cmp(a: TwoTuple, b: TwoTuple) -> bool {
91+
// FIXME-CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
92+
// FIXME-CHECK-DAG: %[[CMP0:.+]] = icmp sle i16 %[[A0]], %[[B0]]
93+
// FIXME-CHECK-DAG: %[[CMP1:.+]] = icmp ule i16 %[[A1]], %[[B1]]
94+
// FIXME-CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
95+
// FIXME-CHECK: ret i1 %[[R]]
96+
Ord::cmp(&a, &b).is_le()
97+
}
98+
99+
// CHECK-LABEL: @check_gt_via_cmp
100+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
101+
#[no_mangle]
102+
pub fn check_gt_via_cmp(a: TwoTuple, b: TwoTuple) -> bool {
103+
// FIXME-CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
104+
// FIXME-CHECK-DAG: %[[CMP0:.+]] = icmp sgt i16 %[[A0]], %[[B0]]
105+
// FIXME-CHECK-DAG: %[[CMP1:.+]] = icmp ugt i16 %[[A1]], %[[B1]]
106+
// FIXME-CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
107+
// FIXME-CHECK: ret i1 %[[R]]
108+
Ord::cmp(&a, &b).is_gt()
109+
}
110+
111+
// CHECK-LABEL: @check_ge_via_cmp
112+
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
113+
#[no_mangle]
114+
pub fn check_ge_via_cmp(a: TwoTuple, b: TwoTuple) -> bool {
115+
// FIXME-CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
116+
// FIXME-CHECK-DAG: %[[CMP0:.+]] = icmp sge i16 %[[A0]], %[[B0]]
117+
// FIXME-CHECK-DAG: %[[CMP1:.+]] = icmp uge i16 %[[A1]], %[[B1]]
118+
// FIXME-CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
119+
// FIXME-CHECK: ret i1 %[[R]]
120+
Ord::cmp(&a, &b).is_ge()
121+
}

0 commit comments

Comments
 (0)