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

Remove non-standard functions from OpenLibm #85

Merged
merged 9 commits into from Feb 12, 2015
Merged

Remove non-standard functions from OpenLibm #85

merged 9 commits into from Feb 12, 2015

Conversation

EdSchouten
Copy link
Contributor

Hi Viral,

As mentioned in issue #79, I promised to send you a pull request to remove non-standard functions from OpenLibm, as they are only needed to keep existing implementations working. New implementations shouldn't offer them. This is how far I got.

Thanks,
Ed

I often build the code with -Wmissing-prototypes to ensure that we don't
accidentically pollute the symbol namespace. If we want to provide a
symbol such as isopenlibm(), make sure we also declare it in
<openlibm_math.h>.
- Don't define llabs(). This breaks if llabs() is already a macro, which
  is allowed by the C standard/POSIX. llabs() was introduced in C99, so
  I think we can safely assume it is present on all interesting systems.

- Cast the parameters to fabs() to the floating point type. Clang has
  introduced some interesting warnings that trigger if the arguments to
  fabs*() are not the right type.
ViralBShah pushed a commit that referenced this pull request Feb 12, 2015
Remove non-standard functions from OpenLibm
@ViralBShah ViralBShah merged commit 8d0843a into JuliaMath:master Feb 12, 2015
@ViralBShah
Copy link
Member

Thank you!

@simonbyrne
Copy link
Member

Apparently julia still calls significand and signficandf:
https://github.com/JuliaLang/julia/blob/1ec68b3173146178ec4f8340b7d2490d4164a4b8/base/math.jl#L135-L141

It would be fairly easy to implement these in Julia.

@ViralBShah
Copy link
Member

I hadn't checked which of these we were using, but the 0.4.1 release of openlibm that we use for now still has these, so julia is safe. For the 0.5 release of openlibm, we will have to update julia too.

@nalimilan
Copy link
Contributor

We're also going to have to bump the SOVERSION major number if we remove functions from the ABI. Better do it in this PR before we forget.

@ViralBShah
Copy link
Member

Already done in 763da44

@tkelman
Copy link
Contributor

tkelman commented Nov 4, 2015

We should put the single precision bessel functions back. Julia is using them and it doesn't hurt anything to have them in the library. The fact that a standard doesn't mention them is an omission in the standard. JuliaLang/julia#13868

@ViralBShah
Copy link
Member

I have been putting such things in openspecfun. I guess we could perhaps have a flag in openlibm that allows extra stuff too. However, seems like @yuyichao has a pure julia solution, which is perhaps even better.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 4, 2015

The pure julia implementation is pretty close in performance but is statistically significantly slower than the openlibm or libm implementation (which are the same one AFAICT....) by ~30%. Using muladd bring that down to ~15%.

I think this is a pretty fun exercise and probably also a useful benchmark to have but I also agree with Tony that there's little point of writing this in julia if the C version is already working and widely tested. It might make sense to ship a julia version if it can beat the C performance or if it provides other benefit.

@ViralBShah
Copy link
Member

Agree. Just need a little bit more engg on the openlibm side for this. If we have an extensions flag in openlibm, we can even move rem_pio2 back from openspecfun back here.

@tkelman
Copy link
Contributor

tkelman commented Nov 4, 2015

Hasn't USE_SYSTEM_LIBM been passing? So glibc libm has these single precision bessel fcns? If that's the case I don't see why we would need an extensions flag. We can eventually move this kind of thing to a SpecialFunctions.jl with both Julia and C/Fortran implementations available, for example nolta's translation of Amos would help portability to systems that don't have fortran compilers. I should check whether MSVC supports these single precision Bessel fcns.

@ViralBShah
Copy link
Member

I believe they are not part of the (C?) spec, but perhaps commonly implemented?

The flag should just be a couple of lines in Makefile. So, if we reinstate these, it should be straightforward to have a flag.

@EdSchouten
Copy link
Contributor Author

The double versions are part of the X/Open UNIX specification, not the C standard. Versions for float and long double are not part of any standard. Search for the [XSI] markers on this page:

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/math.h.html

@yuyichao
Copy link
Contributor

yuyichao commented Nov 6, 2015

Versions for float and long double are not part of any standard.

Which is not a very convincing reason for not implementing them or even removing them from an existing library.

@tkelman
Copy link
Contributor

tkelman commented Nov 13, 2015

@ViralBShah no tagging openlibm until single-precision bessel gets put back

@ViralBShah
Copy link
Member

I guess all we need to do is revert 8c8693c

@tkelman
Copy link
Contributor

tkelman commented Nov 13, 2015

And there have been upstream bugfixes in freebsd/freebsd-src@0563b7a and several other commits, but if those bugs aren't causing test failures in Julia then they aren't necessarily release blocking.

@ViralBShah
Copy link
Member

4c8740a fixes this. I'd still like to bring in the EXTENSIONS build flag, but this should be good for now to tag.

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.

None yet

6 participants