Auto merge of rust-lang#122024 - clubby789:remove-spec-option-pe, r=jhpratt

Remove SpecOptionPartialEq

With the recent LLVM bump, the specialization for Option::partial_eq on types with niches is no longer necessary. I kept the manual implementation as it still gives us better codegen than the derive (will look at this seperately).

Also implemented PartialOrd/Ord by hand as it _somewhat_ improves codegen for rust-lang#49892: https://godbolt.org/z/vx5Y6oW4Y

Author: bors

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)

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::<u32>() {
-                quote! {
-                    #gate_rustc_only
-                    impl core::option::SpecOptionPartialEq for #name {
-                        #[inline]
-                        fn eq(l: &Option<Self>, r: &Option<Self>) -> 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 {

library/core/src/option.rs

@@ -558,17 +558,16 @@ use crate::panicking::{panic, panic_str};
 use crate::pin::Pin;
 use crate::{
     cmp, convert, hint, mem,
-    num::NonZero,
     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")]
-#[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<T> {
     /// No value.
     #[lang = "None"]
@@ -2146,83 +2145,52 @@ impl<'a, T> From<&'a mut Option<T>> 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<T> crate::marker::StructuralPartialEq for Option<T> {}
 #[stable(feature = "rust1", since = "1.0.0")]
 impl<T: PartialEq> PartialEq for Option<T> {
     #[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: <https://github.com/llvm/llvm-project/issues/52622>
-///
-/// Once that's fixed, `Option` should go back to deriving `PartialEq`, as
-/// it used to do before <https://github.com/rust-lang/rust/pull/103556>.
-#[unstable(feature = "spec_option_partial_eq", issue = "none", reason = "exposed only for rustc")]
-#[doc(hidden)]
-pub trait SpecOptionPartialEq: Sized {
-    fn eq(l: &Option<Self>, other: &Option<Self>) -> bool;
-}
-
-#[unstable(feature = "spec_option_partial_eq", issue = "none", reason = "exposed only for rustc")]
-impl<T: PartialEq> SpecOptionPartialEq for T {
-    #[inline]
-    default fn eq(l: &Option<T>, r: &Option<T>) -> 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<Self>, r: &Option<Self>) -> 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<u8>;
-    #[stable(feature = "nonzero", since = "1.28.0")] NonZero<u16>;
-    #[stable(feature = "nonzero", since = "1.28.0")] NonZero<u32>;
-    #[stable(feature = "nonzero", since = "1.28.0")] NonZero<u64>;
-    #[stable(feature = "nonzero", since = "1.28.0")] NonZero<u128>;
-    #[stable(feature = "nonzero", since = "1.28.0")] NonZero<usize>;
-    #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero<i8>;
-    #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero<i16>;
-    #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero<i32>;
-    #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero<i64>;
-    #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero<i128>;
-    #[stable(feature = "signed_nonzero", since = "1.34.0")] NonZero<isize>;
-}
-
-#[stable(feature = "nonnull", since = "1.25.0")]
-impl<T> SpecOptionPartialEq for crate::ptr::NonNull<T> {
+// 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<T: PartialOrd> PartialOrd for Option<T> {
     #[inline]
-    fn eq(l: &Option<Self>, r: &Option<Self>) -> 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())
+    fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
+        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 SpecOptionPartialEq for cmp::Ordering {
+impl<T: Ord> Ord for Option<T> {
     #[inline]
-    fn eq(l: &Option<Self>, r: &Option<Self>) -> bool {
-        l.map_or(2, |x| x as i8) == r.map_or(2, |x| x as i8)
+    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,
+        }
     }
 }
 

tests/assembly/option-nonzero-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<NonZero<u32>>, r: Option<NonZero<u32>>) -> bool {
@@ -36,3 +34,42 @@ pub fn non_null_eq(l: Option<NonNull<u8>>, r: Option<NonNull<u8>>) -> bool {
     // CHECK-NEXT: ret i1
     l == r
 }
+
+// CHECK-lABEL: @ordering_eq
+#[no_mangle]
+pub fn ordering_eq(l: Option<Ordering>, r: Option<Ordering>) -> 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<EnumWithNiche>, r: Option<EnumWithNiche>) -> 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<bool>, r: Option<bool>) -> bool {
+//     // FIXME-CHECK: start:
+//     // FIXME-CHECK-NEXT: icmp eq i8
+//     // FIXME-CHECK-NEXT: ret i1
+//     l == r
+// }