Skip to content

Conversation

@xwu
Copy link
Collaborator

@xwu xwu commented Jul 29, 2017

Now that SE-0104 has been revised, it is possible to implement, on FixedWidthInteger, the protocol requirement for conversions from any binary floating point value. This PR provides one such implementation.

@moiseev
Copy link
Contributor

moiseev commented Aug 2, 2017

@swift-ci Please test

Copy link
Contributor

@moiseev moiseev Aug 2, 2017

Choose a reason for hiding this comment

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

I must be missing something. Is (value: 42, exact: false) a possible result? To me it looks like the nullability of value holds enough information.
UPD: Ahh.. I get it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xwu
Copy link
Collaborator Author

xwu commented Aug 2, 2017

@moiseev Where I could use specific guidance for this patch (besides review of its soundness) is use of optimization hints such as _slowPath.

My hunch is that there are places where these hints could improve performance, but I'm not familiar with these facilities and unsure how I'd even go about determining where it would help.

Copy link
Member

Choose a reason for hiding this comment

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

RIP my favorite comment

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @swiftix IIRC this @_sepamtics attribute was mainly to avoid an unnecessary binary size increase. Am I right?
@xwu my first thought is to mark initializers as @inline(always) and _convert as @inlineable The only way to see the impact of these changes though, is to implement a benchmark. I'd say we can land this PR first, add a benchmark in a separate PR and then start playing with improvements.

Copy link
Contributor

Choose a reason for hiding this comment

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

@moiseev Yes, you are right.@semantics was used to avoid an unnecessary binary size increase.

@moiseev
Copy link
Contributor

moiseev commented Aug 22, 2017

@xwu Please refer to the amazing document @milseman has started recently for the description of _fastPath etc. Also, could you please fix the merge conflicts? I think it makes a lot of sense to merge this PR and then work on improving the performance after introducing some benchmarks.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8ecf8bcf6041fb8c1c07511795a52b48584df5e4

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 8ecf8bcf6041fb8c1c07511795a52b48584df5e4

@xwu
Copy link
Collaborator Author

xwu commented Aug 23, 2017

@moiseev Excellent; please test again and merge when you're ready. I have added only the most obvious _slowPath and _fastPath annotations and will leave the benchmarking in a subsequent PR to more capable hands.

@CodaFi
Copy link
Contributor

CodaFi commented Aug 23, 2017

Before that, could you run a rebase to squash this down to as few commits as possible and remove the last merge commit?

Make internal stdlib function public because it is called from stdlib tests

Add some first-thought optimizations
@xwu
Copy link
Collaborator Author

xwu commented Aug 23, 2017

@CodaFi Done.

@milseman
Copy link
Member

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 8ecf8bcf6041fb8c1c07511795a52b48584df5e4

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8ecf8bcf6041fb8c1c07511795a52b48584df5e4

@milseman
Copy link
Member

@swift-ci please test Linux platform

@moiseev moiseev merged commit 5037e07 into swiftlang:master Aug 23, 2017
@xwu xwu deleted the integer-conversion branch August 23, 2017 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants