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

ENH: build also openblas64_ 64-bit symbol-suffixed library #8

Merged
merged 4 commits into from
Dec 17, 2019

Conversation

pv
Copy link
Contributor

@pv pv commented Dec 7, 2019

Build OpenBLAS with 64-bit integer size and symbol suffix '64_' (INTERFACE64=1
SYMBOLSUFFIX=64_).

This is 64-bit BLAS/LAPACK convention avoiding symbol clashes with
32-bit BLAS/LAPACK, originally introduced for Julia. OpenBLAS built
with this setting is also provided by some Linux distributions (e.g.
Fedora's 64-bit openblas packages).

And for clarification: the objconv tool is necessary only for OSX,
where OpenBLAS Makefiles use it for symbol renaming. As noted
in the commits, the sources are from https://www.agner.org/optimize/#objconv
and I checked the diffs vs. a few other places that are distributing
that tool.

c.f. numpy/numpy#15012

Build OpenBLAS with 64-bit integer size and symbol suffix '64_' (INTERFACE64=1
SYMBOLSUFFIX=64_).

This is 64-bit BLAS/LAPACK convention avoiding symbol clashes with
32-bit BLAS/LAPACK, originally introduced for Julia.  OpenBLAS built
with this setting is also provided by some Linux distributions (e.g.
Fedora's 64-bit openblas packages).
Store only the source.zip file.
Copy link
Collaborator

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

I read over the changes---seems ok to me & CI is green.

I can lift the Draft Status and merge, but will wait for your "ok" first.

The binaries won't get uploaded until this is merged since the secret tokens won't work from your fork of the repo.

It seems to me that this is unlikely to break the workflow for the current OpenBLAS binaries & will give us some new 64 bit integer ones we can start downloading for testing (and some day for 64-bit wheels, when ready).

cc @matthew-brett @rgommers @charris

@pv pv marked this pull request as ready for review December 14, 2019 11:40
@pv
Copy link
Contributor Author

pv commented Dec 14, 2019

Yes, I believe this won't change the way the 32-bit OpenBLAS libs are built, and should be OK to merge. The 64-bit Windows and Linux binaries produced I think are fine. The 64-bit OSX binaries I can't check locally, apart from just the symbol names, but they're here if you want to check before merging: https://github.com/pv/openblas-libs/tree/files-osx-x86_64-1

Anyway, getting this merged would be a big help, as it would allow setting up CIs for the 64-bit BLAS (at least for the symbol-suffixed configuration). Otherwise, I think bitrot will eventually set in when someone forgots to use the CBLAS_NAME macros etc.

@mattip
Copy link
Collaborator

mattip commented Dec 17, 2019

Any reason not to merge this?

@tylerjereddy
Copy link
Collaborator

I was going to spot-check that generated binary locally first by running full NumPy suite with it on MacOS.

Copy link
Collaborator

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

I've confirmed the NumPy 64-bit test passes with the MacOS OpenBLAS binary produced here:

NPY_AVAILABLE_MEM=16GB NPY_USE_BLAS_ILP64=1 NPY_BLAS_ILP64_ORDER=openblas64_ NPY_LAPACK_ILP64_ORDER=openblas64_ python runtests.py --mode=full -t "numpy/linalg/tests/test_linalg.py::test_blas64_dot" -- -rsx

1 passed in 37.83s

Full test suite also looks good with same OpenBLAS:

NPY_AVAILABLE_MEM=16GB NPY_USE_BLAS_ILP64=1 NPY_BLAS_ILP64_ORDER=openblas64_ NPY_LAPACK_ILP64_ORDER=openblas64_ python runtests.py --mode=full -- -n 3

10447 passed, 83 skipped, 16 xfailed, 3 xpassed in 303.09s (0:05:03)

Note that I had to do two things to make this work, both of which are par for the course on Mac with our toolchain:

  • use a package manager to provide an older version of gcc to satisfy the libgfortran runtime requirement
  • point DYLD_LIBRARY_PATH at the runtime path of the newly-download OpenBLAS & libgfortran (since it was set to point to something else)

So, merging, thanks for your patience.

@tylerjereddy tylerjereddy merged commit dc05141 into MacPython:master Dec 17, 2019
@mattip
Copy link
Collaborator

mattip commented Dec 17, 2019

Thanks! This is really nice work all around

@pv
Copy link
Contributor Author

pv commented Dec 17, 2019

Would it also be possible to push the OpenBLAS v0.3.7 builds (which is the version numpy CI is currently using)? I guess this involves pushing a commit to master.

@tylerjereddy
Copy link
Collaborator

Let me check if I have sufficient permissions to push a feature branch to the repo & then open a PR from that---I believe that would allow the upload to occur if I understood @matthew-brett correctly a few months ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants