Declare some FloatingPointSign members explicitly for @inlinable #16043
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The compiler can synthesize these, but it doesn't mark them @inlinable, since in the general case they're just a "default" implementation and not "the only implementation forever". But for a two-element enum that's based on a part of IEEE 754, it's probably safe to assume this is the only implementation forever, and that can be important for performance. https://bugs.swift.org/browse/SR-7094
|
@swift-ci Please benchmark |
|
@swift-ci Please test |
stephentyrone
approved these changes
Apr 19, 2018
|
Confirming that we will never want to change these. |
|
Also @natecook1000 I deliberately didn't put any doc comments on these, assuming they'd be mirrored down from the protocols like they were before. Is that correct? |
|
@jrose-apple I don't know the current state of docs inheritance—it hasn't been great lately. I can follow up once these are in… |
slavapestov
reviewed
Apr 19, 2018
| } | ||
|
|
||
| @inlinable | ||
| public static func ==(a: FloatingPointSign, b: FloatingPointSign) -> Bool { |
There was a problem hiding this comment.
We should have a default == witness for RawRepresentable things somewhere
There was a problem hiding this comment.
We probably do, but a client isn't supposed to inline that when using it on someone else's type.
slavapestov
reviewed
Apr 19, 2018
|
|
||
| @inlinable | ||
| public init?(rawValue: Int) { | ||
| switch rawValue { |
There was a problem hiding this comment.
Why don't we synthesize @inlinable members for any frozen RawRepresentable enum?
slavapestov
reviewed
Apr 19, 2018
|
See the discussion in #15589, but the short form is "this is the only such enum; we can decide what to do in the general case later". |
Build comment file:Optimized (O)Regression (23)
Improvement (16)
No Changes (383)
Unoptimized (Onone)Regression (99)
Improvement (12)
No Changes (311)
Hardware Overview |
|
I'm pretty sure these benchmark results are nonsense. None of these things even touch FloatingPointSign, so unless these three functions changed the code size of the standard library so drastically this is just random noise. Why do we have benchmarks with a ±30% swing anyway? *grump* (I was looking for changes in BinaryFloatingPointProperties, but it turns out those are so fast already that you can't tell.) |
|
Yeah, there's something deeply suspicious about those benchmark regressions. |
|
That's in fact interesting. Usually the noise is within 10% |
jrose-apple
added a commit
to jrose-apple/swift
that referenced
this pull request
Apr 21, 2018
…ftlang#16043) The compiler can synthesize these, but it doesn't mark them @inlinable, since in the general case they're just a "default" implementation and not "the only implementation forever". But for a two-element enum that's based on a part of IEEE 754, it's probably safe to assume this is the only implementation forever, and that can be important for performance. https://bugs.swift.org/browse/SR-7094 (cherry picked from commit f6b67f3)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The compiler can synthesize these, but it doesn't mark them @inlinable, since in the general case they're just a "default" implementation and not "the only implementation forever". But for a two-element enum that's based on a part of IEEE 754, it's probably safe to assume this is the only implementation forever, and that can be important for performance.
SR-7094 / rdar://problem/38030106