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

Math functions for Swift #23140

Merged
merged 11 commits into from
Apr 3, 2019
Merged

Math functions for Swift #23140

merged 11 commits into from
Apr 3, 2019

Conversation

stephentyrone
Copy link
Contributor

@stephentyrone stephentyrone commented Mar 6, 2019

This PR implements SE-0246.

We still need to make a decision about how to handle obsoleting functions provided by the platform module. Because most programs pull in Foundation, they transitively pull in the existing names, which means that most functions calls in concrete contexts will get the tgmath functions, not the stdlib implementations. That's OK, but there are a few functions for which the behavior differs for edge cases (most notably pow), and we'd also like to remove these to cut down the overload sets.

rdar://problem/17625344

@stephentyrone
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Mar 6, 2019

Build failed
Swift Test OS X Platform
Git Sha - f17dffb9b557546d631a6f5ec81e9765152c7d5d

@swift-ci
Copy link
Contributor

swift-ci commented Mar 6, 2019

Build failed
Swift Test Linux Platform
Git Sha - f17dffb9b557546d631a6f5ec81e9765152c7d5d

@xwu xwu changed the title [DNM] Elemenatary functions for Swift (aka Mathable) [DNM] Elementary functions for Swift (aka Mathable) Mar 8, 2019
@stephentyrone
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - f17dffb9b557546d631a6f5ec81e9765152c7d5d

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - f17dffb9b557546d631a6f5ec81e9765152c7d5d

@tkremenek
Copy link
Member

@swift-ci clean test

@tkremenek
Copy link
Member

@stephentyrone @airspeedswift what is preventing this from moving forward?

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test Linux Platform
Git Sha - 7cb09f768aa2acbc890364c21abe60ae44d3b9b6

@stephentyrone
Copy link
Contributor Author

stephentyrone commented Apr 2, 2019

@tkremenek Just shaking out fallout from dropping the Math module, specifically what that means for the existing Platform functions. I expect to push this forward today.

More precisely: when we had the Math module, it was possible for Platform to reexport it and preserve source compatibility while obsoleting the existing functions. That isn't possible any longer unless we have it reexport all of the stdlib, which seems like a bad idea. So I'm working on making the minimally invasive change to preserve the behavior of the platform module as much as possible while obsoleting a few specific functions as necessary to resolve ambiguities. This has turned out to be a bit fussier than I originally anticipated.

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test OS X Platform
Git Sha - 7cb09f768aa2acbc890364c21abe60ae44d3b9b6

@stephentyrone
Copy link
Contributor Author

@swift-ci please test

@stephentyrone stephentyrone marked this pull request as ready for review April 2, 2019 19:19
@stephentyrone stephentyrone changed the title [DNM] Elementary functions for Swift (aka Mathable) Math functions for Swift Apr 2, 2019
@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test Linux Platform
Git Sha - 7cb09f768aa2acbc890364c21abe60ae44d3b9b6

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test OS X Platform
Git Sha - 7cb09f768aa2acbc890364c21abe60ae44d3b9b6

@stephentyrone
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test Linux Platform
Git Sha - d54908b1b7cb6ba7b61cc209c8537530a0044fcb

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test OS X Platform
Git Sha - d54908b1b7cb6ba7b61cc209c8537530a0044fcb

Fundamental decision to be made; should implementation hooks present like `Float.exp(_ x: Float) -> Float` or like `Float.Maths.exp(_ x: Float) -> Float`? Having the intermediate namespace to group them as in the second is definitely nicer, but it requires a little bit of extra machinery, and much more importantly, there doesn't seem to be any way to make access to the static `Maths` associatedtype transparent.
This commit implements SE-0246, by adding conformance to Real to the Float, CGFloat, Double, and Float80 types, implemented either in terms of the system's C math library, existing standard library functionality, or LLVM intrinsics. It includes basic test coverage for these new functions, and deprecates and obsoletes *some* existing functionality in the Platform overlay. We still need to make a decision about how to handle the remaining "tgmath" functions, because obsoleting them is technically a source-breaking change (if users have unqualified names like "exp(1)", it's fine, but it would break users who have used qualified names like "Darwin.exp(1)".)
@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test Linux Platform
Git Sha - 44a5821b678e7c06a8bd367e4f8db95ed8f3bdb9

@swift-ci
Copy link
Contributor

swift-ci commented Apr 2, 2019

Build failed
Swift Test OS X Platform
Git Sha - 44a5821b678e7c06a8bd367e4f8db95ed8f3bdb9

@stephentyrone
Copy link
Contributor Author

@swift-ci clean test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 3, 2019

Build failed
Swift Test Linux Platform
Git Sha - 10fd131

@swift-ci
Copy link
Contributor

swift-ci commented Apr 3, 2019

Build failed
Swift Test OS X Platform
Git Sha - 10fd131

@stephentyrone
Copy link
Contributor Author

@swift-ci please smoke test

@stephentyrone
Copy link
Contributor Author

@swift-ci please test.

@swift-ci
Copy link
Contributor

swift-ci commented Apr 3, 2019

Build failed
Swift Test Linux Platform
Git Sha - 10fd131

@swift-ci
Copy link
Contributor

swift-ci commented Apr 3, 2019

Build failed
Swift Test OS X Platform
Git Sha - 10fd131

@stephentyrone
Copy link
Contributor Author

@swift-ci please test.

@swift-ci
Copy link
Contributor

swift-ci commented Apr 3, 2019

Build failed
Swift Test OS X Platform
Git Sha - 45a76fe

@swift-ci
Copy link
Contributor

swift-ci commented Apr 3, 2019

Build failed
Swift Test Linux Platform
Git Sha - 45a76fe

@tkremenek tkremenek merged commit 777750d into swiftlang:master Apr 3, 2019
@stephentyrone stephentyrone deleted the mafs branch April 3, 2019 21:54
@compnerd
Copy link
Member

compnerd commented Apr 3, 2019

@stephentyrone
Copy link
Contributor Author

@compnerd windows doesn't have hypot either?!

@compnerd
Copy link
Member

compnerd commented Apr 3, 2019

@stephentyrone - clearly MSVCRT hates math :-(

Swift.obj : error LNK2019: unresolved external symbol hypotf referenced in function $sSf5hypotyS2f_SftFZ

I think it may be inlined or something, since they claim it is in the library:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/hypot-hypotf-hypotl-hypot-hypotf-hypotl?view=vs-2019

@stephentyrone
Copy link
Contributor Author

stephentyrone commented Apr 3, 2019

@compnerd Can you replace _swift_stdlib_hypotf with a bogus stub on a branch and see if anything else breaks?

You might also be able to call _hypotf instead. Worst-case scenario, I can open-code an implementation for Windows, but let's see if we can use the MSVCRT first.

@compnerd
Copy link
Member

compnerd commented Apr 3, 2019

Yeah, guess we need to switch it over to _hypotf:

    _Check_return_ __inline float __CRTDECL hypotf(_In_ float _X, _In_ float _Y)
    {
        return _hypotf(_X, _Y);
    }

I'll put up a patch momentarily.

/// ```
///
/// All of these are made available as free functions by importing the Math
/// module:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is outdated, I thought?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I still need to update some comments.

stephentyrone added a commit to stephentyrone/swift that referenced this pull request Apr 4, 2019
This reverts commit 777750d, reversing
changes made to 0c8920e.
stephentyrone added a commit that referenced this pull request Apr 4, 2019
* Revert "Merge pull request #23791 from compnerd/you-know-nothing-clang"

This reverts commit 5150981, reversing
changes made to 8fc305c.

* Revert "Merge pull request #23780 from compnerd/math-is-terrible"

This reverts commit 2d7fedd, reversing
changes made to 0205150.

* Revert "Merge pull request #23140 from stephentyrone/mafs"

This reverts commit 777750d, reversing
changes made to 0c8920e.
mackoj added a commit to mackoj/swift-evolution that referenced this pull request Jan 10, 2020
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.

5 participants