From dc37e373292d00572ff884a1a095cc4574d9fe82 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 16 Feb 2023 21:09:46 -0800 Subject: [PATCH 1/3] Add a codegen test for comparisons of 2-tuples of primitives The operators are all overridden in full for tuples, so those parts pass easily, but they're worth pinning. Going via `Ord::cmp`, though, doesn't optimize away for anything but `cmp`+`is_le`. So this leaves `FIXME`s in the tests for the others. --- tests/codegen/comparison-operators-2-tuple.rs | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 tests/codegen/comparison-operators-2-tuple.rs diff --git a/tests/codegen/comparison-operators-2-tuple.rs b/tests/codegen/comparison-operators-2-tuple.rs new file mode 100644 index 0000000000000..0244162e9bf71 --- /dev/null +++ b/tests/codegen/comparison-operators-2-tuple.rs @@ -0,0 +1,118 @@ +// compile-flags: -C opt-level=1 -Z merge-functions=disabled +// min-llvm-version: 15.0 +// only-x86_64 + +#![crate_type = "lib"] + +use std::cmp::Ordering; + +type TwoTuple = (i16, u16); + +// +// The operators are all overridden directly, so should optimize easily. +// + +// CHECK-LABEL: @check_lt_direct +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_lt_direct(a: TwoTuple, b: TwoTuple) -> bool { + // CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP0:.+]] = icmp slt i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP1:.+]] = icmp ult i16 %[[A1]], %[[B1]] + // CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // CHECK: ret i1 %[[R]] + a < b +} + +// CHECK-LABEL: @check_le_direct +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_le_direct(a: TwoTuple, b: TwoTuple) -> bool { + // CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP0:.+]] = icmp sle i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP1:.+]] = icmp ule i16 %[[A1]], %[[B1]] + // CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // CHECK: ret i1 %[[R]] + a <= b +} + +// CHECK-LABEL: @check_gt_direct +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_gt_direct(a: TwoTuple, b: TwoTuple) -> bool { + // CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP0:.+]] = icmp sgt i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP1:.+]] = icmp ugt i16 %[[A1]], %[[B1]] + // CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // CHECK: ret i1 %[[R]] + a > b +} + +// CHECK-LABEL: @check_ge_direct +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_ge_direct(a: TwoTuple, b: TwoTuple) -> bool { + // CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP0:.+]] = icmp sge i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP1:.+]] = icmp uge i16 %[[A1]], %[[B1]] + // CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // CHECK: ret i1 %[[R]] + a >= b +} + +// +// These ones are harder, since there are more intermediate values to remove. +// +// `<` seems to be getting lucky right now, so test that doesn't regress. +// +// The others, however, aren't managing to optimize away the extra `select`s yet. +// See for more about this. +// + +// CHECK-LABEL: @check_lt_via_cmp +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_lt_via_cmp(a: TwoTuple, b: TwoTuple) -> bool { + // CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP0:.+]] = icmp slt i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP1:.+]] = icmp ult i16 %[[A1]], %[[B1]] + // CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // CHECK: ret i1 %[[R]] + Ord::cmp(&a, &b).is_lt() +} + +// CHECK-LABEL: @check_le_via_cmp +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_le_via_cmp(a: TwoTuple, b: TwoTuple) -> bool { + // FIXME-CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // FIXME-CHECK-DAG: %[[CMP0:.+]] = icmp sle i16 %[[A0]], %[[B0]] + // FIXME-CHECK-DAG: %[[CMP1:.+]] = icmp ule i16 %[[A1]], %[[B1]] + // FIXME-CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // FIXME-CHECK: ret i1 %[[R]] + Ord::cmp(&a, &b).is_le() +} + +// CHECK-LABEL: @check_gt_via_cmp +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_gt_via_cmp(a: TwoTuple, b: TwoTuple) -> bool { + // FIXME-CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // FIXME-CHECK-DAG: %[[CMP0:.+]] = icmp sgt i16 %[[A0]], %[[B0]] + // FIXME-CHECK-DAG: %[[CMP1:.+]] = icmp ugt i16 %[[A1]], %[[B1]] + // FIXME-CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // FIXME-CHECK: ret i1 %[[R]] + Ord::cmp(&a, &b).is_gt() +} + +// CHECK-LABEL: @check_ge_via_cmp +// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) +#[no_mangle] +pub fn check_ge_via_cmp(a: TwoTuple, b: TwoTuple) -> bool { + // FIXME-CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] + // FIXME-CHECK-DAG: %[[CMP0:.+]] = icmp sge i16 %[[A0]], %[[B0]] + // FIXME-CHECK-DAG: %[[CMP1:.+]] = icmp uge i16 %[[A1]], %[[B1]] + // FIXME-CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] + // FIXME-CHECK: ret i1 %[[R]] + Ord::cmp(&a, &b).is_ge() +} From 680e21687d5540a425f11d468edcda0ce39c6289 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 16 Feb 2023 23:59:13 -0800 Subject: [PATCH 2/3] Use `partial_cmp` to implement tuple `lt`/`le`/`ge`/`gt` --- library/core/src/tuple.rs | 48 ++++++++++++++----- tests/codegen/comparison-operators-2-tuple.rs | 7 ++- 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/library/core/src/tuple.rs b/library/core/src/tuple.rs index 28275798f751e..0620e7173bc17 100644 --- a/library/core/src/tuple.rs +++ b/library/core/src/tuple.rs @@ -1,7 +1,7 @@ // See src/libstd/primitive_docs.rs for documentation. -use crate::cmp::Ordering::*; -use crate::cmp::*; +use crate::cmp::Ordering::{self, *}; +use crate::mem::transmute; // Recursive macro for implementing n-ary tuple functions and operations // @@ -61,19 +61,19 @@ macro_rules! tuple_impls { } #[inline] fn lt(&self, other: &($($T,)+)) -> bool { - lexical_ord!(lt, $( ${ignore(T)} self.${index()}, other.${index()} ),+) + lexical_ord!(lt, Less, $( ${ignore(T)} self.${index()}, other.${index()} ),+) } #[inline] fn le(&self, other: &($($T,)+)) -> bool { - lexical_ord!(le, $( ${ignore(T)} self.${index()}, other.${index()} ),+) + lexical_ord!(le, Less, $( ${ignore(T)} self.${index()}, other.${index()} ),+) } #[inline] fn ge(&self, other: &($($T,)+)) -> bool { - lexical_ord!(ge, $( ${ignore(T)} self.${index()}, other.${index()} ),+) + lexical_ord!(ge, Greater, $( ${ignore(T)} self.${index()}, other.${index()} ),+) } #[inline] fn gt(&self, other: &($($T,)+)) -> bool { - lexical_ord!(gt, $( ${ignore(T)} self.${index()}, other.${index()} ),+) + lexical_ord!(gt, Greater, $( ${ignore(T)} self.${index()}, other.${index()} ),+) } } } @@ -123,16 +123,38 @@ macro_rules! maybe_tuple_doc { }; } -// Constructs an expression that performs a lexical ordering using method $rel. +#[inline] +const fn ordering_is_some(c: Option, x: Ordering) -> bool { + // FIXME: Just use `==` once that's const-stable on `Option`s. + // This isn't using `match` because that optimizes worse due to + // making a two-step check (`Some` *then* the inner value). + + // SAFETY: There's no public guarantee for `Option`, + // but we're core so we know that it's definitely a byte. + unsafe { + let c: i8 = transmute(c); + let x: i8 = transmute(Some(x)); + c == x + } +} + +// Constructs an expression that performs a lexical ordering using method `$rel`. // The values are interleaved, so the macro invocation for -// `(a1, a2, a3) < (b1, b2, b3)` would be `lexical_ord!(lt, a1, b1, a2, b2, -// a3, b3)` (and similarly for `lexical_cmp`) +// `(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`. macro_rules! lexical_ord { - ($rel: ident, $a:expr, $b:expr, $($rest_a:expr, $rest_b:expr),+) => { - if $a != $b { lexical_ord!($rel, $a, $b) } - else { lexical_ord!($rel, $($rest_a, $rest_b),+) } + ($rel: ident, $ne_rel: ident, $a:expr, $b:expr, $($rest_a:expr, $rest_b:expr),+) => {{ + let c = PartialOrd::partial_cmp(&$a, &$b); + if !ordering_is_some(c, Equal) { ordering_is_some(c, $ne_rel) } + else { lexical_ord!($rel, $ne_rel, $($rest_a, $rest_b),+) } + }}; + ($rel: ident, $ne_rel: ident, $a:expr, $b:expr) => { + // Use the specific method for the last element + PartialOrd::$rel(&$a, &$b) }; - ($rel: ident, $a:expr, $b:expr) => { ($a) . $rel (& $b) }; } macro_rules! lexical_partial_cmp { diff --git a/tests/codegen/comparison-operators-2-tuple.rs b/tests/codegen/comparison-operators-2-tuple.rs index 0244162e9bf71..a9d25e3b53cff 100644 --- a/tests/codegen/comparison-operators-2-tuple.rs +++ b/tests/codegen/comparison-operators-2-tuple.rs @@ -11,6 +11,9 @@ type TwoTuple = (i16, u16); // // The operators are all overridden directly, so should optimize easily. // +// Yes, the `s[lg]t` is correct for the `[lg]e` version because it's only used +// in the side of the select where we know the values are *not* equal. +// // CHECK-LABEL: @check_lt_direct // CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]]) @@ -29,7 +32,7 @@ pub fn check_lt_direct(a: TwoTuple, b: TwoTuple) -> bool { #[no_mangle] pub fn check_le_direct(a: TwoTuple, b: TwoTuple) -> bool { // CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] - // CHECK-DAG: %[[CMP0:.+]] = icmp sle i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP0:.+]] = icmp slt i16 %[[A0]], %[[B0]] // CHECK-DAG: %[[CMP1:.+]] = icmp ule i16 %[[A1]], %[[B1]] // CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] // CHECK: ret i1 %[[R]] @@ -53,7 +56,7 @@ pub fn check_gt_direct(a: TwoTuple, b: TwoTuple) -> bool { #[no_mangle] pub fn check_ge_direct(a: TwoTuple, b: TwoTuple) -> bool { // CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]] - // CHECK-DAG: %[[CMP0:.+]] = icmp sge i16 %[[A0]], %[[B0]] + // CHECK-DAG: %[[CMP0:.+]] = icmp sgt i16 %[[A0]], %[[B0]] // CHECK-DAG: %[[CMP1:.+]] = icmp uge i16 %[[A1]], %[[B1]] // CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]] // CHECK: ret i1 %[[R]] From 4492793e0ddc755b0db3a055fd9a2b9b215c656e Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Fri, 17 Feb 2023 11:46:19 -0800 Subject: [PATCH 3/3] Add a slightly-contrived tuple comparison benchmark --- library/core/benches/lib.rs | 1 + library/core/benches/tuple.rs | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 library/core/benches/tuple.rs diff --git a/library/core/benches/lib.rs b/library/core/benches/lib.rs index e4100120d8252..74ef0949b8af2 100644 --- a/library/core/benches/lib.rs +++ b/library/core/benches/lib.rs @@ -20,6 +20,7 @@ mod ops; mod pattern; mod slice; mod str; +mod tuple; /// Returns a `rand::Rng` seeded with a consistent seed. /// diff --git a/library/core/benches/tuple.rs b/library/core/benches/tuple.rs new file mode 100644 index 0000000000000..d9ff9d0dd9378 --- /dev/null +++ b/library/core/benches/tuple.rs @@ -0,0 +1,22 @@ +use rand::prelude::*; +use test::{black_box, Bencher}; + +#[bench] +fn bench_tuple_comparison(b: &mut Bencher) { + let mut rng = black_box(super::bench_rng()); + + let data = black_box([ + ("core::iter::adapters::Chain", 123_usize), + ("core::iter::adapters::Clone", 456_usize), + ("core::iter::adapters::Copie", 789_usize), + ("core::iter::adapters::Cycle", 123_usize), + ("core::iter::adapters::Flatt", 456_usize), + ("core::iter::adapters::TakeN", 789_usize), + ]); + + b.iter(|| { + let x = data.choose(&mut rng).unwrap(); + let y = data.choose(&mut rng).unwrap(); + [x < y, x <= y, x > y, x >= y] + }); +}