Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a codegen test for comparisons of 2-tuples of primitives #108156

Closed
wants to merge 1 commit into from

Conversation

scottmcm
Copy link
Member

[no compiler nor library changes]

The operators are all overridden in full for tuples, so those parts pass easily, but they're worth pinning.

Going via Ord::cmp, though, doesn't optimize away for anything but cmp+is_le. So this leaves FIXMEs in the tests for the others, referencing #106107.

(I was using this to test whether using Clang's IR pattern for C++20's <=> optimized any better, so figured I might as well check it in for anyone else looking to investigate similar stuff.)

@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2023
// CHECK-LABEL: @check_lt_via_cmp
// CHECK-SAME: (i16 noundef %[[A0:.+]], i16 noundef %[[A1:.+]], i16 noundef %[[B0:.+]], i16 noundef %[[B1:.+]])
#[no_mangle]
pub fn check_lt_via_cmp(a: TwoTuple, b: TwoTuple) -> bool {
Copy link
Member Author

@scottmcm scottmcm Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting check_lt_via_cmp passed on LLVM 15, but not LLVM 14. Will bump the version needed.

EDIT: Done.

The operators are all overridden in full for tuples, so those parts pass easily, but they're worth pinning.

Going via `Ord::cmp`, though, doesn't optimize away for anything but `cmp`+`is_le`.  So this leaves `FIXME`s in the tests for the others.
@scottmcm
Copy link
Member Author

Looks like I might as well just land this via #108157 instead; closing.

@scottmcm scottmcm closed this Feb 23, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2023
…dtolnay

Use `partial_cmp` to implement tuple `lt`/`le`/`ge`/`gt`

In today's implementation, `(A, B)::gt` contains calls to *both* `A::eq` *and* `A::gt`.

That's fine for primitives, but for things like `String`s it's kinda weird -- `(String, usize)::gt` has a call to both `bcmp` and `memcmp` (<https://rust.godbolt.org/z/7jbbPMesf>) because when `bcmp` says the `String`s aren't equal, it turns around and calls `memcmp` to find out which one's bigger.

This PR changes the implementation to instead implement `(A, …, C, Z)::gt` using `A::partial_cmp`, `…::partial_cmp`, `C::partial_cmp`, and `Z::gt`.  (And analogously for `lt`, `le`, and `ge`.)  That way expensive comparisons don't need to be repeated.

Technically this is an observable change on stable, so I've marked it `needs-fcp` + `T-libs-api` and will
r? rust-lang/libs-api

I'm hoping that this will be non-controversial, however, since it's very similar to the observable changes that were made to the derives (rust-lang#81384 rust-lang#98655) -- like those, this only changes behaviour if a type overrode behaviour in a way inconsistent with the rules for the various traits involved.

(The first commit here is rust-lang#108156, adding the codegen test, which I used to make sure this doesn't regress behaviour for primitives.)

Zulip conversation about this change: <https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60.3E.60.20on.20Tuples/near/328392927>.
@scottmcm scottmcm deleted the tuple-cmp-codegen branch November 14, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants