Skip to content

Commit

Permalink
Avoid the unnecessary innermost match in partial_cmp/cmp.
Browse files Browse the repository at this point in the history 
We currently do a match on the comparison of every field in a struct or
enum variant. But the last field has a degenerate match like this:
```
match ::core::cmp::Ord::cmp(&self.y, &other.y) {
    ::core::cmp::Ordering::Equal =>
	::core::cmp::Ordering::Equal,
    cmp => cmp,
},
```
This commit changes it to this:
```
::core::cmp::Ord::cmp(&self.y, &other.y),
```
This is fairly straightforward thanks to the existing `cs_fold1`
function.

The commit also removes the `cs_fold` function which is no longer used.

(Note: there is some repetition now in `cs_cmp` and `cs_partial_cmp`. I
will remove that in a follow-up PR.)
  • Loading branch information
@nnethercote
nnethercote committed Jul 4, 2022
1 parent 2c911dc commit 0ee79f2
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 139 deletions.
21 changes: 16 additions & 5 deletions compiler/rustc_builtin_macros/src/deriving/cmp/ord.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> Bl
// cmp => cmp
// }
//
let expr = cs_fold(
let expr = cs_fold1(
// foldr nests the if-elses correctly, leaving the first field
// as the outermost one, and the last as the innermost.
false,
Expand All @@ -79,15 +79,12 @@ pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> Bl
// ::std::cmp::Ordering::Equal => old,
// cmp => cmp
// }

let new = {
let [other_f] = other_fs else {
cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`");
};

let args =
vec![cx.expr_addr_of(span, self_f), cx.expr_addr_of(span, other_f.clone())];

cx.expr_call_global(span, cmp_path.clone(), args)
};

Expand All @@ -96,7 +93,21 @@ pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> Bl

cx.expr_match(span, new, vec![eq_arm, neq_arm])
},
cx.expr_path(equals_path.clone()),
|cx, args| match args {
Some((span, self_f, other_fs)) => {
let new = {
let [other_f] = other_fs else {
cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`");
};
let args =
vec![cx.expr_addr_of(span, self_f), cx.expr_addr_of(span, other_f.clone())];
cx.expr_call_global(span, cmp_path.clone(), args)
};

new
}
None => cx.expr_path(equals_path.clone()),
},
Box::new(|cx, span, tag_tuple| {
if tag_tuple.len() != 2 {
cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`")
Expand Down
22 changes: 18 additions & 4 deletions compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use crate::deriving::generic::ty::*;
use crate::deriving::generic::*;
use crate::deriving::{path_std, pathvec_std};

use rustc_ast::MetaItem;
use rustc_ast::ptr::P;
use rustc_ast::{Expr, MetaItem};
use rustc_expand::base::{Annotatable, ExtCtxt};
use rustc_span::symbol::{sym, Ident};
use rustc_span::Span;
Expand Down Expand Up @@ -51,7 +52,6 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_
let test_id = Ident::new(sym::cmp, span);
let ordering = cx.path_global(span, cx.std_path(&[sym::cmp, sym::Ordering, sym::Equal]));
let ordering_expr = cx.expr_path(ordering.clone());
let equals_expr = cx.expr_some(span, ordering_expr);

let partial_cmp_path = cx.std_path(&[sym::cmp, sym::PartialOrd, sym::partial_cmp]);

Expand All @@ -68,7 +68,7 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_
// cmp => cmp
// }
//
let expr = cs_fold(
let expr = cs_fold1(
// foldr nests the if-elses correctly, leaving the first field
// as the outermost one, and the last as the innermost.
false,
Expand All @@ -94,7 +94,21 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_

cx.expr_match(span, new, vec![eq_arm, neq_arm])
},
equals_expr,
|cx: &mut ExtCtxt<'_>, args: Option<(Span, P<Expr>, &[P<Expr>])>| match args {
Some((span, self_f, other_fs)) => {
let new = {
let [other_f] = other_fs else {
cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`");
};
let args =
vec![cx.expr_addr_of(span, self_f), cx.expr_addr_of(span, other_f.clone())];
cx.expr_call_global(span, partial_cmp_path.clone(), args)
};

new
}
None => cx.expr_some(span, ordering_expr.clone()),
},
Box::new(|cx, span, tag_tuple| {
if tag_tuple.len() != 2 {
cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`")
Expand Down
25 changes: 0 additions & 25 deletions compiler/rustc_builtin_macros/src/deriving/generic/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1694,31 +1694,6 @@ fn cs_fold_static(cx: &mut ExtCtxt<'_>, trait_span: Span) -> P<Expr> {
cx.span_bug(trait_span, "static function in `derive`")
}

/// Fold the fields. `use_foldl` controls whether this is done
/// left-to-right (`true`) or right-to-left (`false`).
pub fn cs_fold<F>(
use_foldl: bool,
f: F,
base: P<Expr>,
enum_nonmatch_f: EnumNonMatchCollapsedFunc<'_>,
cx: &mut ExtCtxt<'_>,
trait_span: Span,
substructure: &Substructure<'_>,
) -> P<Expr>
where
F: FnMut(&mut ExtCtxt<'_>, Span, P<Expr>, P<Expr>, &[P<Expr>]) -> P<Expr>,
{
match *substructure.fields {
EnumMatching(.., ref all_fields) | Struct(_, ref all_fields) => {
cs_fold_fields(use_foldl, f, base, cx, all_fields)
}
EnumNonMatchingCollapsed(..) => {
cs_fold_enumnonmatch(enum_nonmatch_f, cx, trait_span, substructure)
}
StaticEnum(..) | StaticStruct(..) => cs_fold_static(cx, trait_span),
}
}

/// Function to fold over fields, with three cases, to generate more efficient and concise code.
/// When the `substructure` has grouped fields, there are two cases:
/// Zero fields: call the base case function with `None` (like the usual base case of `cs_fold`).
Expand Down
128 changes: 23 additions & 105 deletions src/test/ui/deriving/deriving-all-codegen.stdout
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,7 @@ impl ::core::cmp::PartialOrd for Point {
-> ::core::option::Option<::core::cmp::Ordering> {
match ::core::cmp::PartialOrd::partial_cmp(&self.x, &other.x) {
::core::option::Option::Some(::core::cmp::Ordering::Equal) =>
match ::core::cmp::PartialOrd::partial_cmp(&self.y, &other.y)
{
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&self.y, &other.y),
cmp => cmp,
}
}
Expand All @@ -179,11 +173,7 @@ impl ::core::cmp::Ord for Point {
fn cmp(&self, other: &Point) -> ::core::cmp::Ordering {
match ::core::cmp::Ord::cmp(&self.x, &other.x) {
::core::cmp::Ordering::Equal =>
match ::core::cmp::Ord::cmp(&self.y, &other.y) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&self.y, &other.y),
cmp => cmp,
}
}
Expand Down Expand Up @@ -323,13 +313,7 @@ impl ::core::cmp::PartialOrd for Big {
&other.b7) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
match ::core::cmp::PartialOrd::partial_cmp(&self.b8,
&other.b8) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&self.b8, &other.b8),
cmp => cmp,
},
cmp => cmp,
Expand Down Expand Up @@ -365,11 +349,7 @@ impl ::core::cmp::Ord for Big {
::core::cmp::Ordering::Equal =>
match ::core::cmp::Ord::cmp(&self.b7, &other.b7) {
::core::cmp::Ordering::Equal =>
match ::core::cmp::Ord::cmp(&self.b8, &other.b8) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&self.b8, &other.b8),
cmp => cmp,
},
cmp => cmp,
Expand Down Expand Up @@ -461,11 +441,7 @@ impl ::core::cmp::PartialOrd for Packed {
-> ::core::option::Option<::core::cmp::Ordering> {
let Self(__self_0_0) = *self;
let Self(__self_1_0) = *other;
match ::core::cmp::PartialOrd::partial_cmp(&__self_0_0, &__self_1_0) {
::core::option::Option::Some(::core::cmp::Ordering::Equal) =>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
}
::core::cmp::PartialOrd::partial_cmp(&__self_0_0, &__self_1_0)
}
}
#[automatically_derived]
Expand All @@ -475,10 +451,7 @@ impl ::core::cmp::Ord for Packed {
fn cmp(&self, other: &Packed) -> ::core::cmp::Ordering {
let Self(__self_0_0) = *self;
let Self(__self_1_0) = *other;
match ::core::cmp::Ord::cmp(&__self_0_0, &__self_1_0) {
::core::cmp::Ordering::Equal => ::core::cmp::Ordering::Equal,
cmp => cmp,
}
::core::cmp::Ord::cmp(&__self_0_0, &__self_1_0)
}
}

Expand Down Expand Up @@ -621,13 +594,7 @@ impl ::core::cmp::PartialOrd for Enum1 {
match (&*self, &*other) {
(&Enum1::Single { x: ref __self_0 }, &Enum1::Single {
x: ref __arg_1_0 }) =>
match ::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&*__self_0, &*__arg_1_0),
}
}
}
Expand All @@ -639,11 +606,7 @@ impl ::core::cmp::Ord for Enum1 {
match (&*self, &*other) {
(&Enum1::Single { x: ref __self_0 }, &Enum1::Single {
x: ref __arg_1_0 }) =>
match ::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0),
}
}
}
Expand Down Expand Up @@ -873,26 +836,16 @@ impl ::core::cmp::PartialOrd for Mixed {
if __self_vi == __arg_1_vi {
match (&*self, &*other) {
(&Mixed::R(ref __self_0), &Mixed::R(ref __arg_1_0)) =>
match ::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0),
(&Mixed::S { d1: ref __self_0, d2: ref __self_1 },
&Mixed::S { d1: ref __arg_1_0, d2: ref __arg_1_1 }) =>
match ::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
match ::core::cmp::PartialOrd::partial_cmp(&*__self_1,
&*__arg_1_1) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&*__self_1,
&*__arg_1_1),
cmp => cmp,
},
_ =>
Expand All @@ -913,20 +866,12 @@ impl ::core::cmp::Ord for Mixed {
if __self_vi == __arg_1_vi {
match (&*self, &*other) {
(&Mixed::R(ref __self_0), &Mixed::R(ref __arg_1_0)) =>
match ::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0),
(&Mixed::S { d1: ref __self_0, d2: ref __self_1 },
&Mixed::S { d1: ref __arg_1_0, d2: ref __arg_1_1 }) =>
match ::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0) {
::core::cmp::Ordering::Equal =>
match ::core::cmp::Ord::cmp(&*__self_1, &*__arg_1_1) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&*__self_1, &*__arg_1_1),
cmp => cmp,
},
_ => ::core::cmp::Ordering::Equal,
Expand Down Expand Up @@ -1054,29 +999,14 @@ impl ::core::cmp::PartialOrd for Fielded {
if __self_vi == __arg_1_vi {
match (&*self, &*other) {
(&Fielded::X(ref __self_0), &Fielded::X(ref __arg_1_0)) =>
match ::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0),
(&Fielded::Y(ref __self_0), &Fielded::Y(ref __arg_1_0)) =>
match ::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0),
(&Fielded::Z(ref __self_0), &Fielded::Z(ref __arg_1_0)) =>
match ::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0) {
::core::option::Option::Some(::core::cmp::Ordering::Equal)
=>
::core::option::Option::Some(::core::cmp::Ordering::Equal),
cmp => cmp,
},
::core::cmp::PartialOrd::partial_cmp(&*__self_0,
&*__arg_1_0),
_ => unsafe { ::core::intrinsics::unreachable() }
}
} else {
Expand All @@ -1094,23 +1024,11 @@ impl ::core::cmp::Ord for Fielded {
if __self_vi == __arg_1_vi {
match (&*self, &*other) {
(&Fielded::X(ref __self_0), &Fielded::X(ref __arg_1_0)) =>
match ::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0),
(&Fielded::Y(ref __self_0), &Fielded::Y(ref __arg_1_0)) =>
match ::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0),
(&Fielded::Z(ref __self_0), &Fielded::Z(ref __arg_1_0)) =>
match ::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0) {
::core::cmp::Ordering::Equal =>
::core::cmp::Ordering::Equal,
cmp => cmp,
},
::core::cmp::Ord::cmp(&*__self_0, &*__arg_1_0),
_ => unsafe { ::core::intrinsics::unreachable() }
}
} else { ::core::cmp::Ord::cmp(&__self_vi, &__arg_1_vi) }
Expand Down

0 comments on commit 0ee79f2

Please sign in to comment.