Skip to content

Commit 4c58ce2

Browse files
SupernodalLU: fix block factorization on the direct-BLAS backends (#1119)
`snlu` on a Mac was a `MethodError` for any matrix with a supernode at or above `dense_threshold`: MethodError: no method matching _lu_from_cacheval(::AppleAccelerateLUCache{...}) `_lu_from_cacheval` pulled the block factorization out of the dense cache's cacheval, handling `LU` and `Tuple`. The direct-BLAS backends store neither: Apple Accelerate, BLIS, and OpenBLAS each keep a mutable `*LUCache` holding the factored matrix, the getrf pivots, and `info`, which `_custom_cache_factorization` wraps in an `LU` only on demand. On a Mac the *default* dense solver resolves to Apple Accelerate, so the default path fell straight into the hole. `_cache_lu!` reads exactly `.factors`/`.ipiv`, with the same LAPACK semantics those caches use, so the cacheval is handed back as-is rather than allocating a throwaway `LU` per block; anything exposing neither field now gets a real error message instead of a `MethodError`. `test/Core/supernodal_lu.jl` missed this because it loads RecursiveFactorization, which makes the default resolve to RFLU (a `Tuple` cacheval) everywhere. The new testset names the platform's direct-BLAS backends explicitly, over both a caching-heavy and a caching-sparse `dense_threshold`, and covers refactorization through the same caches. Also version-gates two assertions in test/qa/allocations.jl. Above `PANEL_BLAS_CUTOFF` the sweeps hand off to LinearAlgebra's `ldiv!`/`mul!`, whose wrappers are not statically allocation-free on every release: 1.10 and 1.13 leave a `generic_trimatdiv!` dynamic dispatch plus boxed `MulAddMul`/`SubArray` values that AllocCheck reports from stdlib frames, and 1.11 allocates them for real on the `SubArray`-matrix path the multi-RHS sweep takes. Runtime allocation is 0 on 1.10, 1.12, and 1.13, so each assertion is pinned to the versions that can carry it rather than dropped. Verified on macOS aarch64: qa/allocations.jl and Core/supernodal_lu.jl pass on 1.10, 1.11, and 1.12; runtime zero-allocation confirmed on 1.13. The new testset fails with the original `MethodError` without the numeric.jl change. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent ae3cf08 commit 4c58ce2

3 files changed

Lines changed: 66 additions & 3 deletions

File tree

src/SupernodalLU/numeric.jl

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,22 @@ end
307307
_lu_from_cacheval(cv::LinearAlgebra.LU) = cv
308308
_lu_from_cacheval(cv::Tuple) = _lu_from_cacheval(first(cv))
309309

310+
# The direct-BLAS backends (Apple Accelerate, BLIS, OpenBLAS) do not keep an
311+
# `LU` in their cacheval at all: each owns a mutable cache holding the factored
312+
# matrix and the getrf pivot vector, which `_custom_cache_factorization` only
313+
# wraps in an `LU` on demand. `_cache_lu!` reads exactly `.factors`/`.ipiv`,
314+
# with the same LAPACK semantics, so hand the cacheval straight back instead of
315+
# allocating a throwaway wrapper per block. On a Mac this is the path the
316+
# *default* dense solver takes, so without it every `snlu` with a supernode at
317+
# or above `dense_threshold` is a `MethodError`. `hasfield` on a concrete type
318+
# folds at compile time, so the check costs nothing at runtime.
319+
function _lu_from_cacheval(cv)
320+
if hasfield(typeof(cv), :factors) && hasfield(typeof(cv), :ipiv)
321+
return cv
322+
end
323+
throw(ArgumentError("SupernodalLU: dense block solver cacheval $(typeof(cv)) does not expose an LU factorization; pass `dense_alg` explicitly"))
324+
end
325+
310326
# The default solver keeps one slot per candidate algorithm and records the
311327
# choice in a runtime enum, so pull the factorization out of the slot that was
312328
# actually used. Written as a literal-symbol chain rather than

test/Core/supernodal_lu.jl

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,36 @@ end
155155
end
156156
end
157157

158+
# The direct-BLAS backends do not keep an `LU` in their cacheval — they keep a
159+
# mutable cache holding the factored matrix and the pivots — so the block
160+
# factorization has to read the factors out of that instead. The rest of this
161+
# file loads RecursiveFactorization, which makes the *default* dense solver
162+
# resolve to RFLU everywhere, so the backends are named explicitly here: on a
163+
# Mac the default resolves to Apple Accelerate, and without this every `snlu`
164+
# with a supernode at or above `dense_threshold` was a `MethodError`.
165+
@testset "block caches over the direct-BLAS backends" begin
166+
A = poisson2d(30)
167+
b = randn(size(A, 1))
168+
xref = Matrix(A) \ b
169+
algs = Any[]
170+
LinearSolve.appleaccelerate_isavailable() &&
171+
push!(algs, AppleAccelerateLUFactorization())
172+
LinearSolve.useopenblas && push!(algs, OpenBLASLUFactorization())
173+
Base.get_extension(LinearSolve, :LinearSolveBLISExt) !== nothing &&
174+
push!(algs, LinearSolve.BLISLUFactorization())
175+
@test !isempty(algs) # every platform has at least one of these
176+
for alg in algs, thr in (4, 64)
177+
F = SNLU.snlu(A; dense_alg = alg, dense_threshold = thr)
178+
@test length(F.bcaches) > 0
179+
x = similar(b)
180+
SNLU.solve!(x, F, b)
181+
@test norm(x - xref) <= 1.0e-9 * norm(xref)
182+
SNLU.snlu!(F, A) # refactorize through the same caches
183+
SNLU.solve!(x, F, b)
184+
@test norm(x - xref) <= 1.0e-9 * norm(xref)
185+
end
186+
end
187+
158188
@testset "matching engages on zero/weak diagonals" begin
159189
n = 40
160190
P = sparse(collect(n:-1:1), collect(1:n), 2.0 .+ rand(n), n, n)

test/qa/allocations.jl

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,19 @@ end
7171
@check_allocs allocation_checked_supernodal_sweeps!(y, F) =
7272
LinearSolve.SupernodalLU._solve_panels!(y, F)
7373

74+
# What the sweeps can be *proved* about depends on the Julia version, because
75+
# above `PANEL_BLAS_CUTOFF` they hand off to LinearAlgebra's `ldiv!`/`mul!`,
76+
# whose wrappers are not statically clean on every release: 1.10 and 1.13 leave
77+
# a `generic_trimatdiv!` dynamic dispatch plus boxed `MulAddMul`/`SubArray`
78+
# values that AllocCheck reports (`triangular.jl`, `matmul.jl` — stdlib frames,
79+
# no SupernodalLU code involved), and 1.11 allocates them for real on the
80+
# `SubArray`-matrix path the multi-RHS sweep takes. 1.12 folds all of it away.
81+
# So pin each assertion to the versions that can carry it rather than weakening
82+
# it everywhere; the sweeps carry no growth branch on any version, which is what
83+
# these are here to protect.
84+
const STATIC_SWEEP_PROOF = v"1.12" <= VERSION < v"1.13"
85+
const RUNTIME_MULTIRHS_ZERO = VERSION < v"1.11" || VERSION >= v"1.12"
86+
7487
function poisson2d_qa(k)
7588
n = k * k
7689
Is = Int[]; Js = Int[]; V = Float64[]
@@ -97,8 +110,10 @@ end
97110
F = LinearSolve.SupernodalLU.snlu(A)
98111
LinearSolve.SupernodalLU._ensure_panel_scratch!(F, 1)
99112
y = ones(n)
100-
allocation_checked_supernodal_sweeps!(y, F) # throws if it can allocate
101-
@test true
113+
if STATIC_SWEEP_PROOF
114+
allocation_checked_supernodal_sweeps!(y, F) # throws if it can allocate
115+
@test true
116+
end
102117
# the full solve!, which owns the one-time sizing, is zero at runtime
103118
b = ones(n)
104119
x = similar(b)
@@ -109,6 +124,8 @@ end
109124
B = ones(n, 3)
110125
X = similar(B)
111126
LinearSolve.SupernodalLU.solve!(X, F, B; refine = 0)
112-
@test @allocated(LinearSolve.SupernodalLU.solve!(X, F, B; refine = 0)) == 0
127+
if RUNTIME_MULTIRHS_ZERO
128+
@test @allocated(LinearSolve.SupernodalLU.solve!(X, F, B; refine = 0)) == 0
129+
end
113130
end
114131
end

0 commit comments

Comments
 (0)