Skip to content

Commit 9f82651

Browse files
borsbors
committed
Auto merge of #103659 - clubby789:improve-partialord-derive, r=nagisa
Special-case deriving `PartialOrd` for enums with dataless variants I was able to get slightly better codegen by flipping the derived `PartialOrd` logic for two-variant enums. I also tried to document the implementation of the derive macro to make the special-case logic a little clearer. ```rs #[derive(PartialEq, PartialOrd)] pub enum A<T> { A, B(T) } ``` ```diff impl<T: ::core::cmp::PartialOrd> ::core::cmp::PartialOrd for A<T> { #[inline] fn partial_cmp( &self, other: &A<T>, ) -> ::core::option::Option<::core::cmp::Ordering> { let __self_tag = ::core::intrinsics::discriminant_value(self); let __arg1_tag = ::core::intrinsics::discriminant_value(other); - match ::core::cmp::PartialOrd::partial_cmp(&__self_tag, &__arg1_tag) { - ::core::option::Option::Some(::core::cmp::Ordering::Equal) => { - match (self, other) { - (A::B(__self_0), A::B(__arg1_0)) => { - ::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0) - } - _ => ::core::option::Option::Some(::core::cmp::Ordering::Equal), - } + match (self, other) { + (A::B(__self_0), A::B(__arg1_0)) => { + ::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0) } - cmp => cmp, + _ => ::core::cmp::PartialOrd::partial_cmp(&__self_tag, &__arg1_tag), } } } ``` Godbolt: [Current](https://godbolt.org/z/GYjEzG1T8), [New](https://godbolt.org/z/GoK78qx15) I'm not sure how common a case comparing two enums like this (such as `Option`) is, and if it's worth the slowdown of adding a special case to the derive. If it causes overall regressions it might be worth just manually implementing this for `Option`.
2 parents 1e22541 + 2883148 commit 9f82651

File tree

2 files changed

+96
-37
lines changed

2 files changed

+96
-37
lines changed

‎compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs

+73-9
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::deriving::generic::ty::*;
22
use crate::deriving::generic::*;
33
use crate::deriving::{path_std, pathvec_std};
4-
use rustc_ast::MetaItem;
4+
use rustc_ast::{ExprKind, ItemKind, MetaItem, PatKind};
55
use rustc_expand::base::{Annotatable, ExtCtxt};
66
use rustc_span::symbol::{sym, Ident};
77
use rustc_span::Span;
@@ -21,6 +21,27 @@ pub fn expand_deriving_partial_ord(
2121

2222
let attrs = thin_vec![cx.attr_word(sym::inline, span)];
2323

24+
// Order in which to perform matching
25+
let tag_then_data = if let Annotatable::Item(item) = item
26+
&& let ItemKind::Enum(def, _) = &item.kind {
27+
let dataful: Vec<bool> = def.variants.iter().map(|v| !v.data.fields().is_empty()).collect();
28+
match dataful.iter().filter(|&&b| b).count() {
29+
// No data, placing the tag check first makes codegen simpler
30+
0 => true,
31+
1..=2 => false,
32+
_ => {
33+
(0..dataful.len()-1).any(|i| {
34+
if dataful[i] && let Some(idx) = dataful[i+1..].iter().position(|v| *v) {
35+
idx >= 2
36+
} else {
37+
false
38+
}
39+
})
40+
}
41+
}
42+
} else {
43+
true
44+
};
2445
let partial_cmp_def = MethodDef {
2546
name: sym::partial_cmp,
2647
generics: Bounds::empty(),
@@ -30,7 +51,7 @@ pub fn expand_deriving_partial_ord(
3051
attributes: attrs,
3152
fieldless_variants_strategy: FieldlessVariantsStrategy::Unify,
3253
combine_substructure: combine_substructure(Box::new(|cx, span, substr| {
33-
cs_partial_cmp(cx, span, substr)
54+
cs_partial_cmp(cx, span, substr, tag_then_data)
3455
})),
3556
};
3657

@@ -47,7 +68,12 @@ pub fn expand_deriving_partial_ord(
4768
trait_def.expand(cx, mitem, item, push)
4869
}
4970

50-
pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
71+
fn cs_partial_cmp(
72+
cx: &mut ExtCtxt<'_>,
73+
span: Span,
74+
substr: &Substructure<'_>,
75+
tag_then_data: bool,
76+
) -> BlockOrExpr {
5177
let test_id = Ident::new(sym::cmp, span);
5278
let equal_path = cx.path_global(span, cx.std_path(&[sym::cmp, sym::Ordering, sym::Equal]));
5379
let partial_cmp_path = cx.std_path(&[sym::cmp, sym::PartialOrd, sym::partial_cmp]);
@@ -74,12 +100,50 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_
74100
let args = vec![field.self_expr.clone(), other_expr.clone()];
75101
cx.expr_call_global(field.span, partial_cmp_path.clone(), args)
76102
}
77-
CsFold::Combine(span, expr1, expr2) => {
78-
let eq_arm =
79-
cx.arm(span, cx.pat_some(span, cx.pat_path(span, equal_path.clone())), expr1);
80-
let neq_arm =
81-
cx.arm(span, cx.pat_ident(span, test_id), cx.expr_ident(span, test_id));
82-
cx.expr_match(span, expr2, vec![eq_arm, neq_arm])
103+
CsFold::Combine(span, mut expr1, expr2) => {
104+
// When the item is an enum, this expands to
105+
// ```
106+
// match (expr2) {
107+
// Some(Ordering::Equal) => expr1,
108+
// cmp => cmp
109+
// }
110+
// ```
111+
// where `expr2` is `partial_cmp(self_tag, other_tag)`, and `expr1` is a `match`
112+
// against the enum variants. This means that we begin by comparing the enum tags,
113+
// before either inspecting their contents (if they match), or returning
114+
// the `cmp::Ordering` of comparing the enum tags.
115+
// ```
116+
// match partial_cmp(self_tag, other_tag) {
117+
// Some(Ordering::Equal) => match (self, other) {
118+
// (Self::A(self_0), Self::A(other_0)) => partial_cmp(self_0, other_0),
119+
// (Self::B(self_0), Self::B(other_0)) => partial_cmp(self_0, other_0),
120+
// _ => Some(Ordering::Equal)
121+
// }
122+
// cmp => cmp
123+
// }
124+
// ```
125+
// If we have any certain enum layouts, flipping this results in better codegen
126+
// ```
127+
// match (self, other) {
128+
// (Self::A(self_0), Self::A(other_0)) => partial_cmp(self_0, other_0),
129+
// _ => partial_cmp(self_tag, other_tag)
130+
// }
131+
// ```
132+
// Reference: https://github.com/rust-lang/rust/pull/103659#issuecomment-1328126354
133+
134+
if !tag_then_data
135+
&& let ExprKind::Match(_, arms) = &mut expr1.kind
136+
&& let Some(last) = arms.last_mut()
137+
&& let PatKind::Wild = last.pat.kind {
138+
last.body = expr2;
139+
expr1
140+
} else {
141+
let eq_arm =
142+
cx.arm(span, cx.pat_some(span, cx.pat_path(span, equal_path.clone())), expr1);
143+
let neq_arm =
144+
cx.arm(span, cx.pat_ident(span, test_id), cx.expr_ident(span, test_id));
145+
cx.expr_match(span, expr2, vec![eq_arm, neq_arm])
146+
}
83147
}
84148
CsFold::Fieldless => cx.expr_some(span, cx.expr_path(equal_path.clone())),
85149
},

‎tests/ui/deriving/deriving-all-codegen.stdout

+23-28
Original file line numberDiff line numberDiff line change
@@ -889,23 +889,20 @@ impl ::core::cmp::PartialOrd for Mixed {
889889
-> ::core::option::Option<::core::cmp::Ordering> {
890890
let __self_tag = ::core::intrinsics::discriminant_value(self);
891891
let __arg1_tag = ::core::intrinsics::discriminant_value(other);
892-
match ::core::cmp::PartialOrd::partial_cmp(&__self_tag, &__arg1_tag) {
893-
::core::option::Option::Some(::core::cmp::Ordering::Equal) =>
894-
match (self, other) {
895-
(Mixed::R(__self_0), Mixed::R(__arg1_0)) =>
896-
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
897-
(Mixed::S { d1: __self_0, d2: __self_1 }, Mixed::S {
898-
d1: __arg1_0, d2: __arg1_1 }) =>
899-
match ::core::cmp::PartialOrd::partial_cmp(__self_0,
900-
__arg1_0) {
901-
::core::option::Option::Some(::core::cmp::Ordering::Equal)
902-
=> ::core::cmp::PartialOrd::partial_cmp(__self_1, __arg1_1),
903-
cmp => cmp,
904-
},
905-
_ =>
906-
::core::option::Option::Some(::core::cmp::Ordering::Equal),
892+
match (self, other) {
893+
(Mixed::R(__self_0), Mixed::R(__arg1_0)) =>
894+
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
895+
(Mixed::S { d1: __self_0, d2: __self_1 }, Mixed::S {
896+
d1: __arg1_0, d2: __arg1_1 }) =>
897+
match ::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0)
898+
{
899+
::core::option::Option::Some(::core::cmp::Ordering::Equal)
900+
=> ::core::cmp::PartialOrd::partial_cmp(__self_1, __arg1_1),
901+
cmp => cmp,
907902
},
908-
cmp => cmp,
903+
_ =>
904+
::core::cmp::PartialOrd::partial_cmp(&__self_tag,
905+
&__arg1_tag),
909906
}
910907
}
911908
}
@@ -1019,18 +1016,16 @@ impl ::core::cmp::PartialOrd for Fielded {
10191016
-> ::core::option::Option<::core::cmp::Ordering> {
10201017
let __self_tag = ::core::intrinsics::discriminant_value(self);
10211018
let __arg1_tag = ::core::intrinsics::discriminant_value(other);
1022-
match ::core::cmp::PartialOrd::partial_cmp(&__self_tag, &__arg1_tag) {
1023-
::core::option::Option::Some(::core::cmp::Ordering::Equal) =>
1024-
match (self, other) {
1025-
(Fielded::X(__self_0), Fielded::X(__arg1_0)) =>
1026-
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
1027-
(Fielded::Y(__self_0), Fielded::Y(__arg1_0)) =>
1028-
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
1029-
(Fielded::Z(__self_0), Fielded::Z(__arg1_0)) =>
1030-
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
1031-
_ => unsafe { ::core::intrinsics::unreachable() }
1032-
},
1033-
cmp => cmp,
1019+
match (self, other) {
1020+
(Fielded::X(__self_0), Fielded::X(__arg1_0)) =>
1021+
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
1022+
(Fielded::Y(__self_0), Fielded::Y(__arg1_0)) =>
1023+
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
1024+
(Fielded::Z(__self_0), Fielded::Z(__arg1_0)) =>
1025+
::core::cmp::PartialOrd::partial_cmp(__self_0, __arg1_0),
1026+
_ =>
1027+
::core::cmp::PartialOrd::partial_cmp(&__self_tag,
1028+
&__arg1_tag),
10341029
}
10351030
}
10361031
}

0 commit comments

Comments
 (0)