BrianGladman / mpir

Multiple Precision Integers and Rationals
GNU General Public License v3.0
75 stars 36 forks source link

makefile and header fixes for Unix #6

Closed dimpase closed 6 years ago

dimpase commented 6 years ago

@wbhart - here it is

wbhart commented 6 years ago

This PR is against Brian's repo (which is fine). I can't merge it there. Of course I'm happy to merge it from Brian (he can make a PR against the main MPIR repo when he is happy, and I'll merge it).

wbhart commented 6 years ago

The other option of course is to change Brian's repo to the official one. The only job I'm doing at present is merging PRs (and trying to keep broken code out).

dimpase commented 6 years ago

On OSX mpn/t-gcdext test fails. (Linux tests all pass)

dimpase commented 6 years ago

The other option of course is to change Brian's repo to the official one

As I already mentioned, it perhaps is better to make an MPIR Github organisation, and host the main repo there. This would make it more professional-looking, and properly reflect that it's not an individual project.

wbhart commented 6 years ago

Yes, you could do that. I think the previous proposal was that it be made a part of Nemocas, which we don't want. But there's no reason not to have an MPIR GitHub organisation.

I had a look at the OSX logs. There's some warnings there which all look serious. But I don't see any obvious cause for the test failure. It might be worth capturing the test program log (it's piped to a file by MPIR), in case it is a real (heisen) bug.

By the way, Brian added other files (that aren't specific to Windows) on the Windows side. Were they added also on the *nix side? Autotools doesn't pick them up automatically, so they won't even make it into the tarball when you do make dist.

BrianGladman commented 6 years ago

Great work Dima!

BrianGladman commented 6 years ago

I think Dima's idea of MPIR as a GITHUB project in its own right makes sense.

wbhart commented 6 years ago

I agree. Anyone can set that up. I can change the link on the website if anyone does it.

I would give someone else the password for the website, but I can't. Fredrik may be able to give access to someone else though.

BrianGladman commented 6 years ago

The functions I added are mpf_cmp_z, mpn_zero_p and mpn_perfect_power_p.

I added two other functions, but on Windows only: mpz_get_2exp_d and mpf_get_2exp_d. These do the same work as mpz_get_d_2exp and mpf_get_d_2exp but avoid the problems caused by having pointers to 32-bit types being used to store 64-bit values on Windows x64. I don't think these have any imapct on Linux/GCC as they won't be present.

I have revised the documentation of the Windows build in the documentation and I have also added the new function descriptions except for mpn_pefect_power_p (because this was missing in the GMP documentation).

BrianGladman commented 6 years ago

Sadly I am still seeing failures on Travis :-(

dimpase commented 6 years ago

On Tue, Feb 27, 2018 at 11:26 PM, Brian Gladman notifications@github.com wrote:

Sadly I am still seeing failures on Travis :-(

these are OSX-only. I'll look into it in the coming days.

BrianGladman commented 6 years ago

Thanks Dima - its approaching midnight here - I will continue with anything needed from me in the morning. I guess we still have to think about the GIT sub-module issue. But the use of a single msvc sub-directory has cleaned things up a lot anyway so its less of an issue now (I hope!).

dimpase commented 6 years ago

On Tue, Feb 27, 2018 at 11:20 PM, Brian Gladman notifications@github.com wrote:

The functions I added are mpf_cmp_z, mpn_zero_p and mpn_perfect_power_p.

I added two other functions, but on Windows only mpz_get_2exp_d and mpf_get_2exp_d. These do the same work as mpz_get_d_2exp and mpf_get_d_2exp but avoid the problems caused by having pointers to 32-bit typess being used to store 64-bit values on Windows x64. I don't think these have any imapct on Linux/GCC as they won't be present.

It'd be good to have a list of new files you added, just to make sure they are all packaged.

I have revised the documentation of the Windows build in the documentation and I have also added the new function descriptions except for mpn_pefect_power_p (because this was missing in the GMP documentation).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/BrianGladman/mpir/pull/6#issuecomment-369062974, or mute the thread https://github.com/notifications/unsubscribe-auth/ABN8HI5YOEGIkFMjcwPLAewueCdCySnvks5tZI3CgaJpZM4SVwaj .

BrianGladman commented 6 years ago

OK, I'll do that in the morning when I am more awake than I am now!

BrianGladman commented 6 years ago

Apart from the contents of the msvc sub-directory, I have only added three files: mpf/cmp_z.c, mpn/generic/perfpow.c and mpn/generic/zero_p. I imported the latter from GMP but it is not documented in their manual. However, they use it as a basis for their mpz_perfect_power_p implementation whereas we have our own version of this.

So I am now wondering whether we should (a) not bother with mpn_perfect_power_p, (b) keep both of the mpn and mpz versions, or (c) implement our mpz function by calling the mpn version. I haven't been able to test their relative speed yet but I wonder if anyone hs views on what we should do here?

An issue that I have not done anything about is to add the new functions to the speed program - adding new functions to it is a bit of a mystery to me as I haven't played with it.

dimpase commented 6 years ago

One failure we see on OSX is surely due to mpn/generic/zero_p not being properly entered into autoconf/automake configs. But I don't understand how it should be done, mpn/ seems to have a less straightforward than mpf/ way to deal with this.

@wbhart - can you comment on this?

less tests/mpn/t-gcdext.log 
dyld: lazy symbol binding failed: Symbol not found: ___gmpn_zero_p
  Referenced from: /Users/dimpase/Sage/mpir/.libs/libmpir.23.dylib
  Expected in: flat namespace

dyld: Symbol not found: ___gmpn_zero_p
  Referenced from: /Users/dimpase/Sage/mpir/.libs/libmpir.23.dylib
  Expected in: flat namespace

FAIL t-gcdext (exit status: 134)

We don't see this on Linux as a non-generic implementation is chosen.

wbhart commented 6 years ago

Yes, GMP/MPIR has a custom process for building the mpn part of the library. there are devel docs in the repository which explain how to do all this. The process is necessarily quite complex due to the way assembly functions for multiple arches are dealt with and the complexities of fat binaries and conditional inclusion of certain assembly optimisations.

dimpase commented 6 years ago

can you point out to a file in mpn/generic/ that is used and dealt with properly, which I can use as an example?

wbhart commented 6 years ago

The files are handled differently depending on what kind of function it is. But if the file is only present in generic, the documentation covers this straightforwardly.

As an example, inv_div_q.c is a generic only function. You should be able to grep configure.ac and the Makefiles to see how this is handled.

Of course files don't necessarily just drop straight in to MPIR from GMP. But Brian has surely already made any changes that are necessary, so that it works on the Windows side.

BrianGladman commented 6 years ago

Yes, I have made all the changes that I believe are needed other than those within the Linux/GCC build system.