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

Load the symbols of BLIS when we use this package #20

Closed
wants to merge 1 commit into from

Conversation

amontoison
Copy link

No description provided.

@amontoison
Copy link
Author

amontoison commented Apr 18, 2023

@xrq-phys
Is it ok to recompile blis_jll such that we have two shared libraries?
One LP64 and another one ILP64?

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch coverage: 40.00% and project coverage change: +23.44 🎉

Comparison is base (f453201) 51.55% compared to head (27ebe7b) 75.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #20       +/-   ##
===========================================
+ Coverage   51.55%   75.00%   +23.44%     
===========================================
  Files           7       20       +13     
  Lines         225      400      +175     
===========================================
+ Hits          116      300      +184     
+ Misses        109      100        -9     
Impacted Files Coverage Δ
src/BLIS.jl 71.42% <40.00%> (ø)

... and 18 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xrq-phys
Copy link
Collaborator

Sure. BLIS' lib is not huge. It'd be nice to have two libs.

I'm not perfectly sure about library naming in case of 32+64bit blis_jll. Perhaps compile twice when ${nbits}==64 here?

@amontoison
Copy link
Author

amontoison commented Apr 19, 2023

I opened a PR to generate blis32_jll, but it's easier to maintain if we have the two shared libraries in the same build_tarballs.jl.
The names libopenblas and libopenblas64_ are used for LP64 / ILP64 versions of openblas. We can do something similar.
We just need to be careful and change the soname of one shared library but I already did it here: JuliaPackaging/Yggdrasil#6608

@amontoison
Copy link
Author

amontoison commented Apr 19, 2023

We could also create a common script and keep two jll.
https://github.com/JuliaPackaging/Yggdrasil/tree/master/L/LAPACK

Update: It seems ok to use the name libblis for LP64 and ILP64 versions if they are in two jll.

@jd-foster
Copy link

Having an __init__ that calls to BLAS.lbt_forward might be more appropriately added under the BLIS.BLASInterface module; a user of the other APIs might not expect this change.

@amontoison
Copy link
Author

I opened this PR to have the same behavior with MKL.jl and AppleAccelerate.jl (JuliaLinearAlgebra/AppleAccelerate.jl#58) but I don't want to break something for the users of BLIS.jl.

@xrq-phys
Copy link
Collaborator

BLIS.jl does much more than MKL.jl.

BLIS.jl directly overrides LinearAlgebra.jl functionalities. So like rand(100) * rand(100) operations will use BLIS once using BLIS was called without going through libblastrampoline.

The reason here is to support more generic storage of input matrices: If view(mat, 1:2:end, 1:2:end) goes through LinearAlgebra.jl's backend, either the whole submatrix is copied or BLAS is unused at all. This API-level overriding allows these kind of calls to go into BLIS' extended APIs.

In fact, I'm thinking of separating BLIS.jl into two packages: One provides access to the BLIS API and another one overwrites LinearAlgebra.jl functionalities.

@xrq-phys
Copy link
Collaborator

I just realized that the package aiming at (MKL.jl and AppleAccelerate.jl-like) BLAS-backend switching is already available as a separate one: BLISBLAS.jl

Then for BLIS.jl we'd better focus on overriding LinearAlgebra.jl via trait or reflection. @jd-foster I would like to have your opinion.

@amontoison
Copy link
Author

amontoison commented Apr 19, 2023

For information, I'm just interested to load an LP64 BLIS.
I'm working on an Julia-HSL / HSL_jll.jl package with the HSL team and it requires an LP64 BLAS / LAPACK. Julia doesn't load an LP64 BLAS / LAPACK by default so we need to load one with lbt_forward.
Currently, the available options are OpenBLAS32_jll.jl or MKL_jll.jl. I would like to add an easy way to use BLIS32_jll.jl + LAPACK32_jll.jl.

For the new Apple Accelerate, we will need Julia 1.9 and the version 5.7.0 of LBT.

@amontoison
Copy link
Author

I just checked BLISBLAS.jl and it's exactly what I need. I didn't know that this package was registered!

@jd-foster
Copy link

@amontoison BLISBLAS.jl is quite minimal: https://github.com/JuliaLinearAlgebra/BLISBLAS.jl/blob/main/src/BLISBLAS.jl. It just loads blis_jll and LinearAlgebra and then does your __init__.

Look like you found it while I was typing.

@amontoison
Copy link
Author

amontoison commented Apr 19, 2023

Yep, let's close this PR.
I will open a PR to add blis32_jll in BLISBLAS.jl.

Thanks for the explanations!

@amontoison amontoison closed this Apr 19, 2023
@amontoison amontoison deleted the lbt_blis branch April 19, 2023 06:47
@jd-foster
Copy link

jd-foster commented Apr 19, 2023

I will open a PR to add blis32_jll in BLISBLAS.jl.

I think this is the best course of action.

Note that as you found in JuliaLinearAlgebra/libblastrampoline#117 (comment)
that (only) 157 symbols are loaded when using BLAS.lbt_forward with the libblis library at moment, so there is still some work to do:
cf. JuliaLinearAlgebra/libblastrampoline#36

@jd-foster
Copy link

@xrq-phys You're right to define the scope of the BLIS package to be different from doing the LBT forwarding that is covered by BLISBLAS.jl.

I wonder if there is still some functionality that could be added to make LBT forwarding work too. This should be on demand (i.e. not automatically on package loading) such as when some environment variable is set (e.g BLIS_LBT_FOWARD or similar).

@xrq-phys
Copy link
Collaborator

Created #21

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.

3 participants