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

Use libblastrampoline to forward to a user-defined BLAS at runtime #39455

Merged
merged 6 commits into from
Feb 25, 2021

Conversation

ViralBShah
Copy link
Member

@ViralBShah ViralBShah commented Jan 30, 2021

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 :-).

@ViralBShah ViralBShah added domain:building Build system, or building Julia or its dependencies domain:linear algebra Linear algebra labels Jan 30, 2021
@ViralBShah ViralBShah marked this pull request as draft January 30, 2021 01:39
@ViralBShah ViralBShah force-pushed the vs/trampolineblas branch 2 times, most recently from 05f5a88 to 1541a0b Compare February 13, 2021 22:54
@ViralBShah ViralBShah changed the title Use libblastrampoline and pick a BLAS at runtime [WIP] Use libblastrampoline and pick a BLAS at runtime Feb 15, 2021
@ViralBShah ViralBShah marked this pull request as ready for review February 15, 2021 05:12
@ViralBShah
Copy link
Member Author

I wonder if the fake jll for libblastrampoline_jll is necessary - or if it can be removed.

@ViralBShah
Copy link
Member Author

@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.

@nalimilan
Copy link
Member

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 USE_SYSTEM_BLAS is still supported with this PR, right? Currently I point to the appropriate libraries via LIBBLAS/LIBBLASNAME and LIBLAPACK/LIBLAPACKNAME.

@ViralBShah
Copy link
Member Author

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 LIBBLASPATH, which packagers pass to the Julia build along with the others.

@staticfloat
Copy link
Sponsor Member

staticfloat commented Feb 15, 2021

We should also do a test run (in a separate PR) with something like the following inserted into the LinearAlgebra tests:

using Pkg
Pkg.add("MKL_jll")
using MKL_jll
if MKL_jll.is_available()
    LinearAlgebra.set_blas_lapack_trampoline!(MKL_jll.libmkl_rt_path, MKL_jll.libmkl_rt_path)
end

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 libmkl_rt but instead libmkl_ilp64 or something.

@ViralBShah
Copy link
Member Author

ViralBShah commented Feb 15, 2021

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.

@ViralBShah ViralBShah marked this pull request as draft February 15, 2021 22:46
@ViralBShah
Copy link
Member Author

ViralBShah commented Feb 16, 2021

@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).

@ViralBShah ViralBShah marked this pull request as ready for review February 17, 2021 00:16
@ViralBShah
Copy link
Member Author

This PR is now ready for a broad review. It has been tested with MKL as well - and pretty much passes all tests.

@staticfloat
Copy link
Sponsor Member

@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.

@nalimilan
Copy link
Member

Yes, after adding symlinks LinearAlgebra and SuiteSparse tests pass.

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

Minor changes

NEWS.md Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/LinearAlgebra.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/LinearAlgebra.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/LinearAlgebra.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/LinearAlgebra.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/blas.jl Outdated Show resolved Hide resolved
stdlib/libblastrampoline_jll/src/libblastrampoline_jll.jl Outdated Show resolved Hide resolved
@staticfloat
Copy link
Sponsor Member

Yes, after adding symlinks LinearAlgebra and SuiteSparse tests pass.

Can you share with me your build configuration that fails without symlinks? I kind of thought the current setup wouldn't require symlinks.

@ViralBShah
Copy link
Member Author

Yes, after adding symlinks LinearAlgebra and SuiteSparse tests pass.

Can you share with me your build configuration that fails without symlinks? I kind of thought the current setup wouldn't require symlinks.

#39455 (comment)

@staticfloat
Copy link
Sponsor Member

Thanks Viral. I have replicated the issue and.... let's stick with the symlink-workaround for now. 😅

@staticfloat
Copy link
Sponsor Member

I intend to merge when green.

@staticfloat staticfloat merged commit dc81980 into master Feb 25, 2021
@staticfloat staticfloat deleted the vs/trampolineblas branch February 25, 2021 20:36
@ViralBShah ViralBShah changed the title [WIP] Use libblastrampoline to forward to a user-defined BLAS at runtime Use libblastrampoline to forward to a user-defined BLAS at runtime Feb 25, 2021
@fredrikekre
Copy link
Member

fredrikekre commented Feb 26, 2021

Looks like SuiteSparse checksums are missing after this PR:

@tbenst
Copy link

tbenst commented Jun 1, 2021

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..?

@ViralBShah
Copy link
Member Author

ViralBShah commented Jun 1, 2021

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.

@KristofferC
Copy link
Sponsor Member

In my opinion, things like this are way too scary to backport to a patch release. Look at for example #40787.

@ViralBShah
Copy link
Member Author

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.

@StefanKarpinski
Copy link
Sponsor Member

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:building Build system, or building Julia or its dependencies domain:linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet