From 5f254d8b66f995eaeb5316b2e26ca507ebd5d0c4 Mon Sep 17 00:00:00 2001 From: clubby789 Date: Tue, 5 Mar 2024 13:26:47 +0000 Subject: [PATCH 1/2] Remove `SpecOptionPartialEq` --- compiler/rustc_index_macros/src/lib.rs | 8 +- compiler/rustc_index_macros/src/newtype.rs | 28 ------- library/core/src/option.rs | 81 +++---------------- tests/assembly/option-nonzero-eq.rs | 27 ------- ...ption-nonzero-eq.rs => option-niche-eq.rs} | 43 +++++++++- 5 files changed, 51 insertions(+), 136 deletions(-) delete mode 100644 tests/assembly/option-nonzero-eq.rs rename tests/codegen/{option-nonzero-eq.rs => option-niche-eq.rs} (51%) diff --git a/compiler/rustc_index_macros/src/lib.rs b/compiler/rustc_index_macros/src/lib.rs index 532cac5791e06..015518ae4d688 100644 --- a/compiler/rustc_index_macros/src/lib.rs +++ b/compiler/rustc_index_macros/src/lib.rs @@ -34,13 +34,7 @@ mod newtype; #[proc_macro] #[cfg_attr( feature = "nightly", - allow_internal_unstable( - step_trait, - rustc_attrs, - trusted_step, - spec_option_partial_eq, - min_specialization - ) + allow_internal_unstable(step_trait, rustc_attrs, trusted_step, min_specialization) )] pub fn newtype_index(input: TokenStream) -> TokenStream { newtype::newtype(input) diff --git a/compiler/rustc_index_macros/src/newtype.rs b/compiler/rustc_index_macros/src/newtype.rs index 0b25628b9e195..e5c2ba424834a 100644 --- a/compiler/rustc_index_macros/src/newtype.rs +++ b/compiler/rustc_index_macros/src/newtype.rs @@ -156,32 +156,6 @@ impl Parse for Newtype { } }; - let spec_partial_eq_impl = if let Lit::Int(max) = &max { - if let Ok(max_val) = max.base10_parse::() { - quote! { - #gate_rustc_only - impl core::option::SpecOptionPartialEq for #name { - #[inline] - fn eq(l: &Option, r: &Option) -> bool { - if #max_val < u32::MAX { - l.map(|i| i.as_u32()).unwrap_or(#max_val+1) == r.map(|i| i.as_u32()).unwrap_or(#max_val+1) - } else { - match (l, r) { - (Some(l), Some(r)) => r == l, - (None, None) => true, - _ => false - } - } - } - } - } - } else { - quote! {} - } - } else { - quote! {} - }; - Ok(Self(quote! { #(#attrs)* #[derive(Clone, Copy, PartialEq, Eq, Hash, #(#derive_paths),*)] @@ -283,8 +257,6 @@ impl Parse for Newtype { #step - #spec_partial_eq_impl - impl From<#name> for u32 { #[inline] fn from(v: #name) -> u32 { diff --git a/library/core/src/option.rs b/library/core/src/option.rs index 5027e326a9d7a..4ddf68c12d346 100644 --- a/library/core/src/option.rs +++ b/library/core/src/option.rs @@ -557,8 +557,7 @@ use crate::iter::{self, FromIterator, FusedIterator, TrustedLen}; use crate::panicking::{panic, panic_str}; use crate::pin::Pin; use crate::{ - cmp, convert, hint, mem, - num::NonZero, + convert, hint, mem, ops::{self, ControlFlow, Deref, DerefMut}, slice, }; @@ -568,7 +567,7 @@ use crate::{ #[rustc_diagnostic_item = "Option"] #[lang = "Option"] #[stable(feature = "rust1", since = "1.0.0")] -#[allow(clippy::derived_hash_with_manual_eq)] // PartialEq is specialized +#[allow(clippy::derived_hash_with_manual_eq)] // PartialEq is manually implemented equivalently pub enum Option { /// No value. #[lang = "None"] @@ -2146,86 +2145,26 @@ impl<'a, T> From<&'a mut Option> for Option<&'a mut T> { } } +// Ideally, LLVM should be able to optimize our derive code to this. +// Once https://github.com/llvm/llvm-project/issues/52622 is fixed, we can +// go back to deriving `PartialEq`. #[stable(feature = "rust1", since = "1.0.0")] impl crate::marker::StructuralPartialEq for Option {} #[stable(feature = "rust1", since = "1.0.0")] impl PartialEq for Option { #[inline] fn eq(&self, other: &Self) -> bool { - SpecOptionPartialEq::eq(self, other) - } -} - -/// This specialization trait is a workaround for LLVM not currently (2023-01) -/// being able to optimize this itself, even though Alive confirms that it would -/// be legal to do so: -/// -/// Once that's fixed, `Option` should go back to deriving `PartialEq`, as -/// it used to do before . -#[unstable(feature = "spec_option_partial_eq", issue = "none", reason = "exposed only for rustc")] -#[doc(hidden)] -pub trait SpecOptionPartialEq: Sized { - fn eq(l: &Option, other: &Option) -> bool; -} - -#[unstable(feature = "spec_option_partial_eq", issue = "none", reason = "exposed only for rustc")] -impl SpecOptionPartialEq for T { - #[inline] - default fn eq(l: &Option, r: &Option) -> bool { - match (l, r) { + // Spelling out the cases explicitly optimizes better than + // `_ => false` + match (self, other) { (Some(l), Some(r)) => *l == *r, + (Some(_), None) => false, + (None, Some(_)) => false, (None, None) => true, - _ => false, } } } -macro_rules! non_zero_option { - ( $( #[$stability: meta] $NZ:ty; )+ ) => { - $( - #[$stability] - impl SpecOptionPartialEq for $NZ { - #[inline] - fn eq(l: &Option, r: &Option) -> bool { - l.map(Self::get).unwrap_or(0) == r.map(Self::get).unwrap_or(0) - } - } - )+ - }; -} - -non_zero_option! { - #[stable(feature = "nonzero", since = "1.28.0")] NonZero; - #[stable(feature = "nonzero", since = "1.28.0")] NonZero; - #[stable(feature = "nonzero", since = "1.28.0")] NonZero; - #[stable(feature = "nonzero", since = "1.28.0")] NonZero; - #[stable(feature = "nonzero", since = "1.28.0")] NonZero; - #[stable(feature = "nonzero", since = "1.28.0")] NonZero; - #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero; - #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero; - #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero; - #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero; - #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero; - #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero; -} - -#[stable(feature = "nonnull", since = "1.25.0")] -impl SpecOptionPartialEq for crate::ptr::NonNull { - #[inline] - fn eq(l: &Option, r: &Option) -> bool { - l.map(Self::as_ptr).unwrap_or_else(|| crate::ptr::null_mut()) - == r.map(Self::as_ptr).unwrap_or_else(|| crate::ptr::null_mut()) - } -} - -#[stable(feature = "rust1", since = "1.0.0")] -impl SpecOptionPartialEq for cmp::Ordering { - #[inline] - fn eq(l: &Option, r: &Option) -> bool { - l.map_or(2, |x| x as i8) == r.map_or(2, |x| x as i8) - } -} - ///////////////////////////////////////////////////////////////////////////// // The Option Iterators ///////////////////////////////////////////////////////////////////////////// diff --git a/tests/assembly/option-nonzero-eq.rs b/tests/assembly/option-nonzero-eq.rs deleted file mode 100644 index b04cf63fd7837..0000000000000 --- a/tests/assembly/option-nonzero-eq.rs +++ /dev/null @@ -1,27 +0,0 @@ -//@ revisions: WIN LIN -//@ [WIN] only-windows -//@ [LIN] only-linux -//@ assembly-output: emit-asm -//@ compile-flags: --crate-type=lib -O -C llvm-args=-x86-asm-syntax=intel -//@ only-x86_64 -//@ ignore-sgx - -use std::cmp::Ordering; - -// CHECK-lABEL: ordering_eq: -#[no_mangle] -pub fn ordering_eq(l: Option, r: Option) -> bool { - // Linux (System V): first two arguments are rdi then rsi - // Windows: first two arguments are rcx then rdx - // Both use rax for the return value. - - // CHECK-NOT: mov - // CHECK-NOT: test - // CHECK-NOT: cmp - - // LIN: cmp dil, sil - // WIN: cmp cl, dl - // CHECK-NEXT: sete al - // CHECK-NEXT: ret - l == r -} diff --git a/tests/codegen/option-nonzero-eq.rs b/tests/codegen/option-niche-eq.rs similarity index 51% rename from tests/codegen/option-nonzero-eq.rs rename to tests/codegen/option-niche-eq.rs index f637b1aef9757..8b8044e9b75b8 100644 --- a/tests/codegen/option-nonzero-eq.rs +++ b/tests/codegen/option-niche-eq.rs @@ -1,4 +1,5 @@ //@ compile-flags: -O -Zmerge-functions=disabled +//@ min-llvm-version: 18 #![crate_type = "lib"] #![feature(generic_nonzero)] @@ -7,9 +8,6 @@ use core::cmp::Ordering; use core::ptr::NonNull; use core::num::NonZero; -// See also tests/assembly/option-nonzero-eq.rs, for cases with `assume`s in the -// LLVM and thus don't optimize down clearly here, but do in assembly. - // CHECK-lABEL: @non_zero_eq #[no_mangle] pub fn non_zero_eq(l: Option>, r: Option>) -> bool { @@ -36,3 +34,42 @@ pub fn non_null_eq(l: Option>, r: Option>) -> bool { // CHECK-NEXT: ret i1 l == r } + +// CHECK-lABEL: @ordering_eq +#[no_mangle] +pub fn ordering_eq(l: Option, r: Option) -> bool { + // CHECK: start: + // CHECK-NEXT: icmp eq i8 + // CHECK-NEXT: ret i1 + l == r +} + +#[derive(PartialEq)] +pub enum EnumWithNiche { + A, + B, + C, + D, + E, + F, + G, +} + +// CHECK-lABEL: @niche_eq +#[no_mangle] +pub fn niche_eq(l: Option, r: Option) -> bool { + // CHECK: start: + // CHECK-NEXT: icmp eq i8 + // CHECK-NEXT: ret i1 + l == r +} + +// FIXME: This should work too +// // FIXME-CHECK-lABEL: @bool_eq +// #[no_mangle] +// pub fn bool_eq(l: Option, r: Option) -> bool { +// // FIXME-CHECK: start: +// // FIXME-CHECK-NEXT: icmp eq i8 +// // FIXME-CHECK-NEXT: ret i1 +// l == r +// } From f8fd23a2add24de796b2fab197920d3fd349941a Mon Sep 17 00:00:00 2001 From: clubby789 Date: Tue, 5 Mar 2024 14:51:48 +0000 Subject: [PATCH 2/2] Manually implement `PartialOrd`/`Ord` for `Option` --- library/core/src/option.rs | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/library/core/src/option.rs b/library/core/src/option.rs index 4ddf68c12d346..0083d15efaefb 100644 --- a/library/core/src/option.rs +++ b/library/core/src/option.rs @@ -557,13 +557,13 @@ use crate::iter::{self, FromIterator, FusedIterator, TrustedLen}; use crate::panicking::{panic, panic_str}; use crate::pin::Pin; use crate::{ - convert, hint, mem, + cmp, convert, hint, mem, ops::{self, ControlFlow, Deref, DerefMut}, slice, }; /// The `Option` type. See [the module level documentation](self) for more. -#[derive(Copy, PartialOrd, Eq, Ord, Debug, Hash)] +#[derive(Copy, Eq, Debug, Hash)] #[rustc_diagnostic_item = "Option"] #[lang = "Option"] #[stable(feature = "rust1", since = "1.0.0")] @@ -2165,6 +2165,35 @@ impl PartialEq for Option { } } +// Manually implementing here somewhat improves codegen for +// https://github.com/rust-lang/rust/issues/49892, although still +// not optimal. +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialOrd for Option { + #[inline] + fn partial_cmp(&self, other: &Self) -> Option { + match (self, other) { + (Some(l), Some(r)) => l.partial_cmp(r), + (Some(_), None) => Some(cmp::Ordering::Greater), + (None, Some(_)) => Some(cmp::Ordering::Less), + (None, None) => Some(cmp::Ordering::Equal), + } + } +} + +#[stable(feature = "rust1", since = "1.0.0")] +impl Ord for Option { + #[inline] + fn cmp(&self, other: &Self) -> cmp::Ordering { + match (self, other) { + (Some(l), Some(r)) => l.cmp(r), + (Some(_), None) => cmp::Ordering::Greater, + (None, Some(_)) => cmp::Ordering::Less, + (None, None) => cmp::Ordering::Equal, + } + } +} + ///////////////////////////////////////////////////////////////////////////// // The Option Iterators /////////////////////////////////////////////////////////////////////////////