-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Use libblastrampoline to forward to a user-defined BLAS at runtime #39455
Conversation
05f5a88
to
1541a0b
Compare
1541a0b
to
3301e4d
Compare
4b378ad
to
7da0a6b
Compare
I wonder if the fake jll for |
7da0a6b
to
07dcdc9
Compare
@nalimilan Do see the description above on what this PR accomplishes. I think you are a user of USE_SYSTEM_BLAS, and it would be good to get your perspective. |
Thanks for asking. Yes, I think I (and other packagers) will still need to use the system BLAS/LAPACK to build RPM packages. But IIUC |
The USE_SYSTEM_BLAS infrastructure is still in place, and the idea is to continue having it. It won't work until we add a way for the full path to the system BLAS/LAPACK to be communicated to Julia. I am thinking we add a build option like |
We should also do a test run (in a separate PR) with something like the following inserted into the LinearAlgebra tests:
That way we can test whether we can pass the Julia test suite with MKL as well. Hmm, one thing I haven't done so far is figured out how to set MKL into ILP64 mode. We may need to open not |
Setting MKL for ILP64 or another mode: https://software.intel.com/content/www/us/en/develop/documentation/onemkl-developer-reference-c/top/support-functions/single-dynamic-library-control/mkl-set-interface-layer.html Basically, MKL.jl should set up ILP64 first, and we then set up the forwards for LBT - and it should work out. I'll try to create a separate PR for testing MKL. |
@simonbyrne @andreasnoack @dkarrasch @KristofferC @fredrikekre Pinging for a review. Still some ways to go, but this is getting close. Also #39685 tests these capabilities on MKL (which pass locally in my tests). |
574d722
to
5d04754
Compare
This PR is now ready for a broad review. It has been tested with MKL as well - and pretty much passes all tests. |
2fe9271
to
8972976
Compare
@nalimilan can I get a final check from you that this doesn't cause you extra problems? If it works for you, we're ready to merge. |
Yes, after adding symlinks LinearAlgebra and SuiteSparse tests pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes
Co-authored-by: Curtis Vogt <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
Can you share with me your build configuration that fails without symlinks? I kind of thought the current setup wouldn't require symlinks. |
|
Thanks Viral. I have replicated the issue and.... let's stick with the symlink-workaround for now. 😅 |
I intend to merge when green. |
Looks like SuiteSparse checksums are missing after this PR: |
It would be wonderful to see this apart of the next LTS. Might this this PR be compatible with 1.6? I’m not sure if this would be suitable for 1.6 semvar or not but that would seem one rapid solution..? |
Could certainly be backported, but not sure if @KristofferC will approve. I suppose we should wait for 1.7 to come out, let it all stabilize, and then decide if we want to backport. |
In my opinion, things like this are way too scary to backport to a patch release. Look at for example JuliaLang/LinearAlgebra.jl#845. |
There have been a few more like that, but yes, we at least want to get to 1.7.1 and see if we feel it is stable enough in the wild and then think about backporting, if at all. |
What about not having this feature on the LTS makes life/ecosystem maintenance hard enough to justify backporting? For things like the new manifest format, it would be very painful for people who have to go back and forth between versions, but this seems like something that has a pretty straightforward story of "use the newer version if you want this feature". |
Incorporate libblastrampoline (LBT)
LBT is pulled in from BB (and can be built locally too). All BLAS and LAPACK are now routed through LBT in the LinearAlgebra stdlib. SuiteSparse is also linked against LBT, if you build it, and a new SuiteSparse_LBT jll is provided in BB.
OpenBLAS is still installed as before, and is what LBT forwards to by default. Thus LIBBLAS, LIBBLASNAME, LIBLAPACK, and LIBLAPACKNAME remain unchanged. The PR removes all the other MKL/ATLAS support for the Julia build, since these can now be provided through external packages. Ideally, we should move all that openblas configuration stuff to deps/openblas.mk.
One question is - do people still want to build Julia with system provided LIBBLAS and LIBLAPACK? If so, the only issue is to have a mechanism to locate the full paths of those libraries. One possibility is that for people using system provided BLAS/LAPACK, they must do it through a package. The other possibility is that we allow a way to provide/discover the full path at Julia build time.
Also bumps SuiteSparse to 5.8.1 and moves aarch64 to ILP64 BLAS.
Putting this up for early feedback, so that it can be merged early in the 1.7 release cycle. This will allow adequate time for BB to catch up so that all BB libraries then link to LBT, and the user can pick a BLAS of their choice, including possibly a future Julia BLAS :-).