-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[stdlib] Make use of protocol requirements to convert from concrete floating-point types #33803
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
Conversation
This comment has been minimized.
This comment has been minimized.
cc @troughton I figure it's best to start small and chip away at this issue gradually. This should be an easy win. |
@swift-ci benchmark |
@@ -1890,7 +1890,18 @@ extension BinaryFloatingPoint { | |||
/// - Parameter value: A floating-point value to be converted. | |||
@inlinable | |||
public init<Source: BinaryFloatingPoint>(_ value: Source) { | |||
self = Self._convert(from: value).value | |||
switch value { | |||
case let value_ as Float: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should handle Float16
as well (implemented as self = Self(Float(value_))
in that case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentyrone The required checks for the availability of Float16
are...not pretty. See the commit "🤮" for details.
There's a benchmark PR not yet merged, by the way. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c4ef4b6
to
70b68d6
Compare
@swift-ci test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@swift-ci test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@swift-ci test Linux platform |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hmm @swift-ci please benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
This PR is about picking the low-hanging fruit in terms of recovering performance during generic floating-point conversion. It is so low-hanging, in fact, it's actually partly a reversion to the original placeholder implementation of generic floating-point conversion.
In brief, we never removed the requirements for
init(_: Float)
,init(_: Double)
, andinit(_: Float80)
from the protocol, and now that we have ABI stability, these requirements will be present forever. So, we can call them from the generic implementation.There is one caveat to be highlighted: As in other situations (see #30417), the compiler regards the generic initializer itself as a suitable default implementation of
init(_: Float)
and friends, and therefore it will not warn if a concrete type does not provide its own implementations of the latter. Currently, this means that the concrete type will unwittingly have a very slow implementation; with this change, the concrete type will have an implementation that crashes at runtime due to infinite recursion. I think it's arguable as to which one is the worse outcome; this PR doesn't cause or otherwise change the underlying issue.