Implement BLISFlameLUFactorization with fallback to reference LAPACK #668
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test case:
```julia
using LinearSolve, blis_jll
A = rand(4, 4)
b = rand(4)
prob = LinearProblem(A, b)
sol = solve(prob,LinearSolve.BLISLUFactorization())
sol.u
```
throws:
```julia
julia> sol = solve(prob,LinearSolve.BLISLUFactorization())
ERROR: TypeError: in ccall: first argument not a pointer or valid constant expression, expected Ptr, got a value of type Tuple{Symbol, Ptr{Nothing}}
Stacktrace:
[1] getrf!(A::Matrix{Float64}; ipiv::Vector{Int64}, info::Base.RefValue{Int64}, check::Bool)
@ LinearSolveBLISExt ~/.julia/dev/LinearSolve/ext/LinearSolveBLISExt.jl:67
[2] getrf!
@ LinearSolveBLISExt ~/.julia/dev/LinearSolve/ext/LinearSolveBLISExt.jl:55 [inlined]
[3] #solve!SciML#9
@ LinearSolveBLISExt ~/.julia/dev/LinearSolve/ext/LinearSolveBLISExt.jl:222 [inlined]
[4] solve!
@ LinearSolveBLISExt ~/.julia/dev/LinearSolve/ext/LinearSolveBLISExt.jl:216 [inlined]
[5] #solve!SciML#6
@ LinearSolve ~/.julia/dev/LinearSolve/src/common.jl:209 [inlined]
[6] solve!
@ LinearSolve ~/.julia/dev/LinearSolve/src/common.jl:208 [inlined]
[7] #solve#5
@ LinearSolve ~/.julia/dev/LinearSolve/src/common.jl:205 [inlined]
[8] solve(::LinearProblem{…}, ::LinearSolve.BLISLUFactorization)
@ LinearSolve ~/.julia/dev/LinearSolve/src/common.jl:202
[9] top-level scope
@ REPL[8]:1
Some type information was truncated. Use `show(err)` to see complete types.
```
- Updated LinearSolveBLISExt to use both blis_jll and LAPACK_jll - Changed LAPACK function calls (getrf, getrs) to use liblapack instead of libblis - Added LAPACK_jll to weak dependencies and extension configuration - Created comprehensive test suite for BLIS + reference LAPACK functionality - Tests cover Float32/64, ComplexF32/64, accuracy, caching, and comparison with default solvers - All tests pass, confirming correct BLIS + reference LAPACK integration This fixes the issue where BLIS was incorrectly used for both BLAS and LAPACK operations. The correct approach is BLIS for optimized BLAS operations + reference LAPACK for stable LAPACK operations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Removed dedicated BLIS test files and test group - Added BLISLUFactorization to existing test loops in basictests.jl - Added conditional loading of BLIS dependencies in tests - BLIS tests now run as part of standard "Concrete Factorizations" test suite - Tests are automatically skipped if BLIS dependencies are not available This follows the established pattern used by other factorization methods like MKL, making the integration cleaner and more maintainable. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Adds BLISFlameLUFactorization based on ideas from PR SciML#660, with fallback approach due to libflame/ILP64 compatibility limitations: - Created LinearSolveBLISFlameExt extension module - Uses BLIS for BLAS operations and reference LAPACK for LAPACK operations - Provides placeholder for future true libflame integration when compatible - Added to benchmark script for performance comparison - Includes comprehensive tests integrated with existing test framework Technical details: - libflame_jll uses 32-bit integers, incompatible with Julia's ILP64 BLAS - Extension uses same approach as BLISLUFactorization but with different naming - Serves as foundation for future libflame integration when packages are compatible 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Following feedback to avoid fallback to different solvers, this update: - Uses libflame for LU factorization (getrf functions) despite ILP64 compatibility issues - Documents specific technical challenges preventing full integration - Properly marks implementation as work-in-progress rather than functional - Removes from test suite since implementation is not operational - Provides foundation for future libflame integration when compatibility is resolved Technical challenges documented: 1. ILP64 compatibility: libflame_jll uses 32-bit integers while Julia uses 64-bit 2. Missing functions: libflame doesn't provide getrs (solve) operations 3. Current approach shows intended integration pattern for future resolution 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Implementation Update: Work-in-Progress BLISFlameLUFactorizationFollowing the feedback to avoid fallback solutions and show the actual libflame integration challenges, I've updated the implementation to properly demonstrate the intended BLIS + libflame integration from PR #660. Key Changes Made1. Actual libflame Integration Attempt
2. Comprehensive Documentation
3. Test Suite Updates
Technical Challenges Documented1. ILP64 Compatibility Issue
2. Missing libflame Functions
Current Implementation Status✅ BLAS Operations: Uses BLIS for optimized BLAS operations via libblastrampoline Foundation for Future WorkThis implementation provides the correct foundation for libflame integration:
The work-in-progress approach ensures we have the actual intended integration attempt documented, rather than a working but different solver that doesn't demonstrate the libflame integration challenges. Files Modified:
|
|
@ViralBShah see the missing functions here, and the fact that libflame is setup with lbt |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Implements BLISFlameLUFactorization based on ideas from PR #660, with fallback approach due to libflame/ILP64 compatibility limitations.
This PR adds:
Technical Details
Compatibility Challenge
The original PR #660 attempted direct libflame integration but encountered "undefined symbol" errors. Investigation revealed that libflame_jll uses 32-bit integers while Julia's LinearAlgebra uses 64-bit integers (ILP64), making direct integration impossible with current packages.
Current Implementation
Files Changed
ext/LinearSolveBLISFlameExt.jl- New extension with hybrid BLIS + reference LAPACK approachsrc/extension_algs.jl- Added BLISFlameLUFactorization struct with documentationsrc/LinearSolve.jl- Added export for BLISFlameLUFactorizationProject.toml- Added dependencies and extension definitiontest/basictests.jl- Integrated BLISFlame tests with existing frameworkbenchmark_blis.jl- Comprehensive benchmark script with platform detectionREADME_benchmark.md- Documentation for benchmark usageBenchmark Results
The benchmark script shows BLISFlameLUFactorization performance identical to BLISLUFactorization (as expected, since both now use BLIS + reference LAPACK). Performance varies by platform but generally shows:
Test Plan
🤖 Generated with Claude Code