Implement fast path for `derive(PartialOrd)` when deriving `Ord` by makai410 · Pull Request #155598 · rust-lang/rust

makai410 · 2026-04-21T14:48:26Z

Unfortunately, this PR shares the same issue with #124794, which doesn't work when derive(PartialOrd) is before derive(Ord).

rustbot · 2026-04-21T14:48:29Z

Changes to the code generated for builtin derived traits.

cc @nnethercote

rustbot · 2026-04-21T14:48:32Z

r? @ShoyuVanilla

rustbot has assigned @ShoyuVanilla.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 72 candidates
Random selection from 18 candidates

makai410 · 2026-04-21T14:49:24Z

-                    cx.dcx().span_bug(field.span, "not exactly 2 arguments in `derive(Ord)`");
+                    cx.dcx()
+                        .span_bug(field.span, "not exactly 2 arguments in `derive(PartialOrd)`");
                };


I think this is probably a copy-paste curse, so I changed that, not 100% sure though.

View changes since the review

nnethercote · 2026-04-21T22:43:00Z

I can review this: r? @nnethercote

cc @theemathas, who has made good comments on similar PRs in the past.

makai410 · 2026-04-22T08:37:37Z

@bors try @rust-timer queue

Implement fast path for `derive(PartialOrd)` when deriving `Ord`

petrochenkov · 2026-04-22T11:34:26Z

    resolutions: Vec<DeriveResolution>,
    helper_attrs: Vec<(usize, IdentKey, Span)>,
+    // if this list keeps getting extended, we could use `bitflags`,
+    // something like what [`rustc_type_ir::flags::TypeFlags`] is doing.


We had multiple flags here in the past, and they indeed used flags, and one map in the resolver instead of two containers_deriving_(copy,ord) maps.

View changes since the review

Remove PartialEq and Eq from the SpecialDerives. #65892

Thanks for providing the context. I was thinking about doing one more round of scanning derives to collect all flags such that it might solve #124794. I don't know if that had been discussed before, and if it did I'd really want to know the reason of why not doing that.

one more round of scanning derives to collect all flags

If I correctly understand what you are talking about, it's a much worse hack and it cannot work correctly.
We already doing something like that to support deprecation lint legacy_derive_helpers, and that logic is going to be removed soon.

petrochenkov · 2026-04-22T11:37:14Z

Similarly to the existing derive(Clone, Copy) I'd classify this as adding hacks to the expansion infra, and would recommend against this by default, unless there are really strong performance reasons.

Moreover, the Copy hack can be in theory removed in favor of some MIR transform, but for this one it would be harder to do.

rust-bors · 2026-04-22T14:46:45Z

💔 Test for f5daf9f failed: CI

makai410 · 2026-04-23T05:24:55Z

@bors try

Implement fast path for `derive(PartialOrd)` when deriving `Ord`

rust-bors · 2026-04-23T07:33:41Z

☀️ Try build successful (CI)
Build commit: 803fd19 (803fd197cdeec49615d500f71f89872b51cfc6a9, parent: 30837cb66de032fbc8072c8ecbf76c39c5b14478)

rust-timer · 2026-04-23T08:14:43Z

Finished benchmarking commit (803fd19): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.3%, 0.9%]	3
Regressions ❌ (secondary)	0.9%	[0.7%, 1.0%]	6
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	17
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	2
All ❌✅ (primary)	-0.2%	[-0.4%, 0.9%]	20

Max RSS (memory usage)

Results (primary 0.7%, secondary -3.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.9%	[0.5%, 5.7%]	3
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-2.6%	[-3.2%, -1.9%]	2
Improvements ✅ (secondary)	-6.3%	[-6.5%, -6.1%]	2
All ❌✅ (primary)	0.7%	[-3.2%, 5.7%]	5

Cycles

Results (primary 2.8%, secondary 4.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	4.5%	[3.9%, 5.3%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Binary size

Results (primary 0.1%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.4%]	46
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	24
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.0%]	6
Improvements ✅ (secondary)	-0.1%	[-0.4%, -0.0%]	14
All ❌✅ (primary)	0.1%	[-0.3%, 0.4%]	52

Bootstrap: 490.44s -> 500.217s (1.99%)
Artifact size: 394.27 MiB -> 394.33 MiB (0.01%)

makai410 · 2026-04-24T13:44:06Z

Hmmm, so the perf result doesn't seem to be a strong performance reason? I'm fine with closing this.

nnethercote · 2026-04-25T00:58:37Z

The change isn't necessarily going to speed up the compiler itself. "Generate tight code for derives" is a good goal in general.

rust-bors · 2026-04-26T00:34:35Z

☔ The latest upstream changes (presumably #155796) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 21, 2026

rustbot assigned ShoyuVanilla Apr 21, 2026

makai410 commented Apr 21, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot assigned nnethercote and unassigned ShoyuVanilla Apr 21, 2026

Implement fast path for derive(PartialOrd)

c15a04c

makai410 force-pushed the partial-ord branch from 159a353 to c15a04c Compare April 22, 2026 01:58

theemathas reviewed Apr 22, 2026

View reviewed changes

Comment thread compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs

This comment has been minimized.

Sign in to view

theemathas mentioned this pull request Apr 22, 2026

decl_macro can call pub fn methods of private types. #155631

Open

makai410 added 2 commits April 22, 2026 14:14

Use UFCS for Ord::cmp

2481e2c

Avoid regression in derive(PartialOrd) for unit structs

76ac16f

theemathas reviewed Apr 22, 2026

View reviewed changes

Comment thread tests/ui/deriving/deriving-all-codegen.stdout Outdated

Avoid regression in derive(PartialOrd) for zero-variant enums

2fd68fb

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 22, 2026

rust-bors Bot pushed a commit that referenced this pull request Apr 22, 2026

Auto merge of #155598 - makai410:partial-ord, r=<try>

f5daf9f

Implement fast path for `derive(PartialOrd)` when deriving `Ord`

petrochenkov reviewed Apr 22, 2026

View reviewed changes

rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2026

This comment has been minimized.

Sign in to view

rust-bors Bot pushed a commit that referenced this pull request Apr 23, 2026

Auto merge of #155598 - makai410:partial-ord, r=<try>

803fd19

Implement fast path for `derive(PartialOrd)` when deriving `Ord`

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 23, 2026

Uh oh!

Conversation

makai410 commented Apr 21, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 21, 2026

Uh oh!

rustbot commented Apr 21, 2026

Uh oh!

makai410 Apr 21, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

nnethercote commented Apr 21, 2026

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

makai410 commented Apr 22, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

petrochenkov Apr 22, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

makai410 Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

petrochenkov Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Apr 22, 2026

Uh oh!

rust-bors Bot commented Apr 22, 2026

Uh oh!

makai410 commented Apr 23, 2026

Uh oh!

This comment has been minimized.

rust-bors Bot commented Apr 23, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Apr 23, 2026

Overall result: ❌✅ regressions and improvements - please read:

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

makai410 commented Apr 24, 2026

Uh oh!

nnethercote commented Apr 25, 2026

Uh oh!

rust-bors Bot commented Apr 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

makai410 commented Apr 21, 2026 •

edited by rustbot

Loading

makai410 Apr 21, 2026 •

edited by rustbot

Loading

petrochenkov Apr 22, 2026 •

edited by rustbot

Loading