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

Comparison operators #702

Merged
merged 17 commits into from
Sep 24, 2021
Merged

Comparison operators #702

merged 17 commits into from
Sep 24, 2021

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Aug 4, 2021

This proposal introduces the operators ==, !=, <, <=, >, and >= to Carbon.

@zygoloid zygoloid added the proposal A proposal label Aug 4, 2021
@zygoloid zygoloid requested a review from a team August 4, 2021 00:31
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Aug 4, 2021
This proposal introduces the operators `==`, `!=`, `<`, `<=`, `>`, and
`>=` to Carbon.
@zygoloid zygoloid marked this pull request as ready for review August 4, 2021 02:09
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Aug 4, 2021
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

I'm a bit unsure what the performance implications of the mathematically correct comparison proposal here, particularly when comparing ints with floats. That gives me pause right now, but I think I've noted this in comments.

proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Show resolved Hide resolved
proposals/p0702.md Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
Comment on lines 168 to 171
When both operands are of standard Carbon numeric types (`Int(n)` or
`Float(n)`), no conversions are performed on either operand, and the result is
the mathematically correct result for that comparison, or `False` if either
operand is a NaN. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should consider disallowing == and != when one side is integral and the other is floating-point, because equality comparisons on floating point numbers are in some ways much more like bitwise operations than mathematical operations, and from that point of view there is no "mathematically correct result".

proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
@jonmeow
Copy link
Contributor

jonmeow commented Aug 4, 2021

I think I'm good with this; thumbs up premised on the current #integers discussion.

proposals/p0702.md Outdated Show resolved Hide resolved
comparisons except where the integer type's values are a subset of the
floating-point type's values.
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Looking over this, I assume you've been updating from Thursday's discussion. What you have is pretty much as I recall.

proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Show resolved Hide resolved
Copy link
Contributor Author

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

I've reworked some of the description to try to make it clearer and to rebase on the implicit conversions design in #820.

proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Show resolved Hide resolved
Copy link
Contributor

@josh11b josh11b left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me! I've just suggested a few trivial changes.

proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
proposals/p0702.md Outdated Show resolved Hide resolved
zygoloid and others added 2 commits September 17, 2021 18:15
Co-authored-by: josh11b <[email protected]>
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Resolved some threads that seemed clearly addressed and the folks happy with the results.

There was one thread that I wanted to call out because it got a bit lost due to changes to the section:
#702 (comment)

This is largely about integer and floating point equality comparison. I think the rationale here is solid -- given the implicit conversion rules, I think allowing the comparisons is the least surprising approach. I totally understand that equality comparison with floating point numbers is ... tricky at best, but I don't think the language should be that defensive here.

I think that the mixed-type comparisons is a bold direction ... it may not work out and we may have to do changes there for performance. But I think it is worth attempting given the significant interest in the C++ community itself for attempting the same thing.

So, overall, LGTM, ship it. I've left a trivial comment about benchmark numbers, but I don't think it changes the overall proposal really.

Comment on lines 319 to 320
It is likely that better code could be generated than that currently produced in
this benchmark for these operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

https://quick-bench.com/q/1_xA8G_jXci_yeOKt0WgCc6eGN4 seems to get to 1.3x and 1.4x. ;]

But it's still a "bad" test -- most users aren't conjuring booleans, they're branching. Most of the extra slowness is materializing the 0 and 1 value here. It'll be harder to measure the difference when used in real-world code.

But it's still a difference and so it may be something we have to pay attention to here long-term.

Copy link
Contributor Author

@zygoloid zygoloid Sep 22, 2021

Choose a reason for hiding this comment

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

The calculation I'm using here is:

((correct operation benchmark time) - (no-op benchmark time)) /
((converting operation benchmark time) - (no-op benchmark time))

Using that calculation, I get 2.0x and 2.5x for the two operations in your revised approach rather than the 1.3x and 1.4x as reported by quick-bench. Even though these are similar to the numbers I had before, I've switched to your quick-bench link because I think your branchless implementation seems preferable in general. I previously reported 2.8x and 1.8x, but I'm now realizing that the 1.8x was cheating because it involved a branch that happened to be always taken, that skipped half of the comparison. And while the 2.8x involved a branch that was taken half the time, that was in predictable batches of 500 occurrences branching each way.

I've also added results for a case where we branch on the result of the computation, where the performance delta does seem smaller -- and the delta disappears entirely for one of the two operations; perhaps we're getting one of the two tests for free somehow. I expect that's still not really representative, but perhaps it's closer to being so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh! Good catch on the noop-baseline.

Probably spent too much time on this, but I've got a re-worked benchmark that I like a bit better:
https://godbolt.org/z/xj3n6PKfb

You can't run it on quick-bench.com because it needs Abseil, but it actually uses a controllable distribution of random values. As pasted it should use non-negative values that fit into an i64.

The result is, at least on my Skylake VM, identical performance for the branchless version and the converting version. The branching version behaves a bit differently. Note that these nearly never find "true" for equality test, and so that's a bit biasing here.

If you shift the dataset a bit to, for example, have negative integers then it skews very differently: the non-converting cases are actually much faster. Why? Because they can short circuit by only looking at one input when that input is negative! But we shouldn't expect this to be representative of anything, that's why I skewed the distribution back to make the negative test essentially always fail -- if that test ever succeeds then the converting case will be slower.

It also seems to show much tighter behavior with floating point as well, but I've not spent much time analyzing that.

Happy to iterate a bit here and get a benchmark that we can actually include in the repository if useful (and it seems like it might be) to ensure we're doing a good job of setting expectations here.

But regardless, this shouldn't block the proposal. Feel free to merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, to compile the above locally on Linux, I think the following will work:

brew install google-benchmark abseil
clang++ -o conv_bench conv_bench.cpp -O3 -I(brew --prefix google-benchmark)/include -I(brew --prefix abseil)/include (brew --prefix google-benchmark)/lib/libbenchmark.a (brew --prefix abseil)/lib/libabsl_*.so -lpthread -Wl,-rpath,(brew --prefix abseil)/lib -gmlt

@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Sep 22, 2021
@zygoloid zygoloid merged commit 917386f into carbon-language:trunk Sep 24, 2021
jonmeow added a commit that referenced this pull request Feb 7, 2022
Mostly pulling in the text of #702, but with some small textual edits and adjusting links.


Co-authored-by: Richard Smith <[email protected]>
@zygoloid zygoloid deleted the proposal-comparison-ops branch March 11, 2022 01:02
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
This proposal introduces the operators `==`, `!=`, `<`, `<=`, `>`, and
`>=` to Carbon.

Co-authored-by: Jon Meow <[email protected]>
Co-authored-by: Geoff Romer <[email protected]>
Co-authored-by: josh11b <[email protected]>
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Mostly pulling in the text of #702, but with some small textual edits and adjusting links.


Co-authored-by: Richard Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants