Skip to content

Conversation

@amontoison
Copy link
Member

No description provided.

@amontoison amontoison mentioned this pull request Jun 10, 2024
@ViralBShah ViralBShah marked this pull request as draft October 15, 2025 02:51
@ViralBShah ViralBShah marked this pull request as ready for review October 15, 2025 02:51
@ViralBShah
Copy link
Member

ViralBShah commented Oct 15, 2025

When not cleared, the dot related functions were probably calling openblas. These usually have a return value from the fortran code and are always a bit painful to deal with. The rest looks pretty good.

@ViralBShah
Copy link
Member

ViralBShah commented Oct 15, 2025

The cblas symbols have not been mangled (and it is a bit surprising that tests on linux and windows with Julia 1.12 are still passing in CI - but they do fail locally):

➜  BLIS git:(vs/blis-2.0-mac) nm /Users/viral/.julia/artifacts/0919ab6258c97e07a08f7c98560b1399742102d6/lib/libblis.4.0.0.dylib | grep -i cblas | head -n 10
0000000000148b78 s _CBLAS_CallFromC
00000000000daa70 T _bli_info_get_enable_cblas
00000000001046e4 T _cblas_caxpby
00000000000f75b0 T _cblas_caxpy
00000000000f75e0 T _cblas_ccopy
00000000000f7610 T _cblas_cdotc_sub
00000000000f7640 T _cblas_cdotu_sub
00000000000f7670 T _cblas_cgbmv
00000000000f79f0 T _cblas_cgemm
0000000000104714 T _cblas_cgemm3m

@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.66%. Comparing base (e7e4008) to head (2973371).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           1        1           
  Lines          12       12           
=======================================
  Hits           11       11           
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ViralBShah
Copy link
Member

I have also noticed some issues with triangular solves. There are different errors on Julia 1.10 vs 1.12 - so that could be due to LBT issues.

@ViralBShah
Copy link
Member

cc @imciner2

@ViralBShah
Copy link
Member

I have a potential fix incoming for BLIS to fix the cblas name mangling.

@ViralBShah
Copy link
Member

The CBLAS forwards are fixed in JuliaPackaging/Yggdrasil#12298 which fixes the dot related tests.

The triangular solve stuff needs to come from LAPACK - so this PR by itself is unlikely to go through without #6

@imciner2
Copy link
Contributor

I think the issue with the CBLAS symbols not being picked up is because we autodetect the symbol suffix using BLAS, but then assume it is applied everywhere. I believe you could fix this in the package loading by providing hints to the loader function that it has the non-suffixed symbols. But note that because that won't have the BLAS function with it, we would need to specify all the details about the function interface explicitly (e.g., integer width, complex retval, etc.).

Alternately, I am increasinly wondering if the design of LBT means we can get away with not suffixing the symbols anymore - because we lookup the symbol address in the exact library we want, I think we can tolerate the same symbol names in two different libraries (but I think the libraries definitely need to be named differently to allow the loader to differentiate between them). @giordano @staticfloat do either of you have any thoughts on the name mangling now?

@ViralBShah
Copy link
Member

ViralBShah commented Oct 15, 2025

@imciner2 My latest patches to BLIS fix the CBLAS symbols cleanly for now, and everything has 64_ suffixes. I think we are good to go here.

The triangular solve tests should probably not be in LinearAlgebra's blas.jl tests, since strtrs etc. do not appear to be in all BLAS and seem like should be an LAPACK test.

@ViralBShah
Copy link
Member

Alternately, I am increasinly wondering if the design of LBT means we can get away with not suffixing the symbols anymore - because we lookup the symbol address in the exact library we want, I think we can tolerate the same symbol names in two different libraries (but I think the libraries definitely need to be named differently to allow the loader to differentiate between them). @giordano @staticfloat do either of you have any thoughts on the name mangling now?

Perhaps this should move to the LBT repo so we don't lose the conversation?

@ViralBShah
Copy link
Member

Implemented in #20.

@ViralBShah ViralBShah closed this Oct 15, 2025
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