-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Reimplement floating-point description implementation in Swift. #82750
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
base: main
Are you sure you want to change the base?
Conversation
CC: @stephentyrone for visibility. (I'll eventually need advice about how to deal with the availability mess here.) |
295114c
to
28ca1f0
Compare
// ================================================================ | ||
|
||
// Support Legacy ABI on top of new implementation | ||
@_silgen_name("swift_float32ToString2") |
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 Do we really need to keep these legacy ABIs? The old implementation went to some effort to preserve them, but I'm not sure why.
value d: Float64, | ||
buffer utf8Buffer: inout MutableSpan<UTF8.CodeUnit>) -> Range<Int> | ||
{ | ||
if #available(macOS 9999, *) { |
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.
@al45tair What's the right way to write this dance? It is here only to try to safely call the other functions in this same file. But as written here, it ends up doing a runtime availability check, which I would rather avoid as it's entirely pointless.
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.
You could go with if #available(StdlibDeploymentTarget 6.0)
here and annotate the things it uses with @available(StdlibDeploymentTarget 6.0)
. That should avoid any runtime tests (everywhere, I think, except possibly if one of this is inlineable and we're back-deploying). I picked 6.0 because that corresponds to macOS 15, and I think there are some existing annotations in the file suggesting that version?
28ca1f0
to
5bfe1c1
Compare
value f: Float16, | ||
buffer utf8Buffer: inout MutableSpan<UTF8.CodeUnit>) -> Range<Int> | ||
{ | ||
if #available(SwiftStdlib 6.2, *) { |
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.
Does this necessarily do a runtime OS version check? How does one avoid that?
In effect, we are re-implementing an old API using new ones. The new ones are always "available" in the sense that they're always compiled -- the availability on the new APIs is only there to inform clients of their back-deployment options. (Our availability notions seem to mistakenly conflate "when something was introduced" with "what requirements it has".)
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.
It won't do a runtime version check if the target that we're building for is high enough — it doesn't always do it.
Entirely coincidentally, I was thinking this morning (before I'd seen this comment) that we really should have both @available()
and @requires()
and that we were really conflating exactly those two notions.
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.
Perhaps we should add @introduced()
and @requires()
and document that @available()
implies both of the others? That would give a migration path.
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 seems to be missing the addition of the new file to swiftCore in the new runtimes build.
How do I fix that? I'm not familiar with "the new runtimes build." |
Add swift/Runtimes/Core/core/CMakeLists.txt Line 82 in e690f1d
|
5b20b0b
to
cac5983
Compare
@swift-ci Please test |
1 similar comment
@swift-ci Please test |
@etcwilde Now we're failing in CI for AVR. Clearly, a 16-bit platform does not want this file. How do I exclude it from those? |
@swift-ci Please test |
I tweaked one place where 32-bit int was assumed but not specified. This may let it build on AVR. |
32-bit WASM tests revealed a problem with Float32 formatting on 32-bit CPUs. There also seems to be some problem with Float64 and NaNs that I'm trying to track down. |
@swift-ci Please test Linux |
1 similar comment
@swift-ci Please test Linux |
Note: This is still incomplete. I need to finish porting the Float16 and Float80 support before it can fully replace the existing C implementation.
eefe4ff
to
deccd0a
Compare
@swift-ci Please test |
@natecook1000 I'm toying with the idea of restructuring this a bit. Basically, making the new source file directly implement |
return unsafe buffer.withBytes { (bufferPtr) in | ||
unsafe String._fromASCII( | ||
UnsafeBufferPointer(start: bufferPtr, count: length)) | ||
if #available(SwiftStdlib 6.2, *) { |
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.
@tshortli @al45tair -- In my local testing, it appears that this check results in a call to __isPlatformVersionAtLeast
which takes a full 1/3 of the run time for debugDescription
. This shouldn't be necessary at all -- the implementation here has no dependencies outside of the standard library that we're part of. How can we eliminate this?
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.
Do you get a different outcome if you use _unreachable()
instead of fatalError()
?
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.
Local stdlib builds set the deployment target to something way too old, see 144244211
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.
You could also use StdlibDeploymentTarget 6.2
here (but as before, you might end up having to move more things to StdlibDeploymentTarget 6.2
as a result). That will get rid of the check, because when you build locally, you'll be testing for the target you're building for.
That would let you test the performance sensibly here, I think.
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.
You can override the default deployment target when building locally by passing something like --darwin-deployment-version-osx 26
to build-script
, if you just want to look at performance locally.
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.
build-script should really do this by default, because the alternative is even less representative of the way the stdlib actually gets built.
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.
Preach.
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.
You could also use StdlibDeploymentTarget 6.2 here (but as before, you might end up having to move more things to StdlibDeploymentTarget 6.2 as a result).
Nope. That doesn't work for InlineArray
, at least not with a stock build-script -rA
invocation, since "value generics are only available on macOS 26." I discussed this in #83710. If I landed that, then I could duplicate InlineArray
to make internal _InlineArray
which would not need any availability restrictions at all. Using the same pattern to create internal _MutableSpan
would probably just eliminate all of the availability checks in this code.
I've not experimented with _unreachable()
yet.
I'm playing with build-script incantations to see what other options exist.
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.
... something like --darwin-deployment-version-osx 26 to build-script, if you just want to look at performance locally
Sure, but that still misses the point: Nothing in this code requires macOS 26. Everything it needs is in libswiftCore. The runtime host version is irrelevant.
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.
It would let you look at performance locally without the version checks getting in the way. It doesn’t solve the larger issue, of course.
unsafe String._fromASCII( | ||
UnsafeBufferPointer(start: bufferPtr, count: length)) | ||
if #available(SwiftStdlib 6.2, *) { | ||
var buffer = InlineArray<64, UTF8.CodeUnit>(repeating: 0) |
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.
Any reason not to initialize this to 0x30
instead of zero?
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.
A-ha! Nice catch indeed! Yes, that would eliminate some work later on.
For example, this avoids the need to do any explicit byte-by-byte writes when expanding "123" out to "123000000.0". This also required reworking the "back out extra digits" process for Float64 to ensure the unused digits get written as '0' characters instead of null bytes.
This replaces the previous SwiftDtoa.cpp with the same algorithm reimplemented in Swift. It supports Float16, Float32, Float64, and Float80 (on Intel).
Performance is reasonable: In my testing (M1 and x86_64): Float16 and Float32 are a bit faster than the C version, Float64 is almost exactly the same, Float80 is a bit slower.
I think I've finally worked out the availability, though I'd appreciate someone who knows better taking a critical look.