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

Fix Base.libblas_name/Base.liblapack_name #48435

Merged
merged 4 commits into from
Feb 3, 2023

Conversation

giordano
Copy link
Contributor

On Windows they need to include the major soversion of libblastrampoline.

Fix JuliaLang/LinearAlgebra.jl#981.

@giordano giordano added the linear algebra Linear algebra label Jan 27, 2023
base/Base.jl Outdated
Comment on lines 191 to 192
const libblas_name = "libblastrampoline" * (Sys.iswindows() ? "-5" : "")
const liblapack_name = libblas_name
Copy link
Member

Choose a reason for hiding this comment

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

Can we somehow mark this as deprecated and instead tell this list of packages that they should use libblastrampoline_jll.libblastrampoline instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you deprecate a variable (that is, with a nice warning when you try to access it)? And I'm not sure that's a future-proof option, in case we come up with yet another blas idea in the future, Base.libblas_name looks more agnostic.

@giordano
Copy link
Contributor Author

I added a test for the use of Base.libblas_name, to avoid regressing again like in JuliaLang/LinearAlgebra.jl#981.

@giordano
Copy link
Contributor Author

Ehr, does anyone know what would cause

      From worker 12:	Exception: EXCEPTION_ACCESS_VIOLATION at 0x5fed810 -- dot_compute at C:\buildkite-agent\builds\win2k22-amdci6-1\julialang\julia-master\julia-86e64109fb\bin\libopenblas64_.dll (unknown line)
      From worker 12:	in expression starting at C:\buildkite-agent\builds\win2k22-amdci6-1\julialang\julia-master\julia-86e64109fb\share\julia\stdlib\v1.10\LinearAlgebra\test\blas.jl:719
      From worker 12:	dot_compute at C:\buildkite-agent\builds\win2k22-amdci6-1\julialang\julia-master\julia-86e64109fb\bin\libopenblas64_.dll (unknown line)
      From worker 12:	ddot_k_ZEN at C:\buildkite-agent\builds\win2k22-amdci6-1\julialang\julia-master\julia-86e64109fb\bin\libopenblas64_.dll (unknown line)

This is in the newly added test, it fails only on 64-bit Windows....

@giordano
Copy link
Contributor Author

Using the 64-bit Windows build in this PR, I tried to run

using LinearAlgebra, Libdl
dot_sym = dlsym(dlopen(Base.libblas_name), "cblas_ddot" * (Sys.WORD_SIZE == 64 ? "64_" : ""))
@ccall $(dot_sym)(2::Cint, [2.0, 3.0]::Ref{Cdouble}, 1::Cint, [4.0, 5.0]::Ref{Cdouble}, 1::Cint)::Cdouble

under Wine and it worked fine for me, so I have no clue of what's going on here. I'm happy to change the test to something else if someone can suggest something which doesn't crash on Windows.

@giordano giordano added the backport 1.9 Change should be backported to release-1.9 label Jan 30, 2023
@giordano giordano added this to the 1.9 milestone Jan 30, 2023
@KristofferC KristofferC mentioned this pull request Feb 1, 2023
35 tasks
@staticfloat
Copy link
Member

The problem is that on 64-bit systems, you need to pass the integers as Int64, not Cint. Why this works on some platforms and not others is more of an accident around calling conventions and values that just so happen to be zero, than anything else.

On Windows they need to include the major soversion of libblastrampoline.
Use `Int` as type for integer arguments, instead of `Cint`.
@giordano
Copy link
Contributor Author

giordano commented Feb 3, 2023

That worked, thanks! netlib documentation isn't very helpful, I found the signature of cblas_ddot at https://netlib.org/lapack/explore-html/db/d91/cblas__ddot_8c_source.html, where the integer arguments have type CBLAS_INT which is

#define CBLAS_INT   int32_t

🤷

@staticfloat
Copy link
Member

Right, but that’s for an LP64 build. ILP64 means defining CBLSS_INT to be int64_t.

I myself have caused such issues dozens of times in the past. It’s easy to forget. :P

@staticfloat staticfloat merged commit d02516e into JuliaLang:master Feb 3, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Feb 7, 2023
@giordano giordano deleted the mg/libblas branch February 10, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BandedMatrices Nightly CI failure on Windows with 'could not load library "libblastrampoline"'
3 participants