Repository navigation
T17: report - #80
T17: report#80
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f7cbd8e to
a7d2107
Compare
| view(N.info, (ioff + 1):(ioff + count)), count) | ||
| ka_chol_check_info!(N.info, ioff, F11) | ||
| else | ||
| _potrf_batched_impl!(p.bpotrf, F11, N.info, ioff) |
There was a problem hiding this comment.
nit (non-blocking): the :ka batched potrf goes through ka_potrf! on a 3-D array, which launches with a 2-D ndrange (WG, nb) (src/dense/fallback/potrf.jl:79). On the KA CPU backend that allocates per launch (the same reason the Report gives for flattening the batch index into 1-D ndranges everywhere else), so the batched regime-C path with impl = :auto/:ka on the CPU backend is not allocation-free, unlike the single-member path. GPUs are unaffected. Suggest a follow-up: a 1-D _ka_potrf_kernel! variant (group index decoded as (WG, member)) so the CPU budget tests can cover batched regime C too.
| _set_members!(solver) | ||
| reduce_stats!(N, S) |
There was a problem hiding this comment.
nit (non-blocking): for Cholesky, getparam("inertia"/"pivot_stats"/"npivots") on a batch re-reduces stats only for the members selected by the current ubatch_index/ubatch_mask, but the returned vector covers all nbatch members. The inactive members' entries come from whatever totals held after the last reduction (zeros if they were never active). That matches "inactive members are left untouched", but it is worth a sentence in the getparam docstring's uniform-batch paragraph so a caller reading one member's inertia after a masked refactorization knows the others may be stale.
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE (head a7d2107)
Reviewed T17 (uniform batch) against the task text in TASKS.md, AGENTS.md and PLAN.md §1.3, §1.4, §1.6, §3.5.
Scope. Only the task's deliverables: batch stride in Numeric (panels interleaved per front, info interleaved per entry, the rest member-major), a BatchMap argument replacing first in every numeric and solve kernel with the member index flattened into the 1-D ndrange, regime C over runs of consecutive members with strided-batched potrf/trsm/gemm and vendor pointer arrays built once at allocation, ubatch_size/ubatch_index/ubatch_mask, per-member info/inertia/npivots/pivot_stats, batched RHS layouts and MatrixDescriptor(T, n[, nrhs]; nbatch), generic auto-detection. PLAN.md and Manifest.toml untouched; TASKS.md changed only in the T17 section (marker [!], Report). Public names, parameter strings and phase strings unchanged. Backend code stays in ext/; the three new vendor bindings are declared in src/dense/vendor.jl with NotSupportedError defaults.
Tests. test/test_ubatch.jl covers every listed case: nb ∈ {1,2,3,16,64} × nrhs ∈ {1,2,4} for SPD and "S"; ubatch_index/ubatch_mask with a bitwise comparison of the untouched members' factor, D, piv, kinds, stats and info; strided / matrix / 3-D / descriptor layouts giving identical solutions; one non-SPD member with info[k] == column, 0 elsewhere, across regimes A+B, B, C; LinearAlgebra layer and error paths. test/ported/cudss_uniform_batch.jl ports the LDLᴴ and Cholesky parts of test_uniform_batch_cudss.jl (LU skipped, T19, as the task allows). Shared helpers were added in test/matrices.jl, test/utils.jl, test/backends.jl; loops over BACKENDS, ELTYPES (and INTTYPES in the port); tol(T); Random.seed!(666) from the driver. The three test_api.jl assertions that encoded "not until T17" now assert the implemented behaviour and are listed in the Report. No @test_broken added.
Conventions. Generic in T and INT; the batch overflow check (nb · factor_len < typemax(INT), nb · (ns+1) < typemax(Int32)) runs on the host before the device arrays are built. 1-D workgroups, @localmem sized by Val, no atomics in assembly/extend-add/pack_add! (each packed entry of each member is written by exactly one work item), dense ops through the interface, no @allowscalar, errors from src/errors.jl, docstrings on new public functions.
Correctness (read closely). MemberPanels is only ever indexed at s, never used as P[s+1] - P[s]; _zero_front! was changed to use f·w accordingly and layout.jl:186 confirms panels are exactly f·w and contiguous. amap offsets stay in the single-matrix layout and are shifted by _member_shift; the regime-A _subtree_scatter! keeps the single-layout front_ptr (local buffer offsets) while _subtree_write!/_lt_subtree_write! get member panels. info reads/writes (_iview, (s-1)·nb + k, ns·nb + k) are consistent across regimes A/B/C, cholesky_stats! and the host read. Solve: compact columns map to user columns through _bm_ucol, the per-member piv/d/pivot_kind views and the member panels are selected by _bm_cmember; the two kernels that still receive the single-matrix front_ptr (_sv_pull_kernel!, _sv_piv_kernel!) never read the factor. Refinement norms are indexed 2r-1, 2r on compact columns and read with ncols. trsm side/trans flags and gemm with transB = 'C' (real diagonal forced in the batched pack_add!) match the single path's herk. d_len = 2n matches the hard-coded stride in _factor_diag. ubatch_mask is a host Vector{Int} option, so active_members does no scalar device indexing.
Report. Claims match the diff; deviations (vendor trsm per member in the solve, transposed batched RHS and logabsdet of a batch unsupported, pivot pairs from member 1, the test_api.jl assertion changes) are listed. Test counts are plausible and not contradicted by CI: build is green, the CPU and CUDA test jobs are still pending at review time (handled by the pipeline). I could not run Julia in this session, so the counts are taken from the Report pending CI.
Blocking findings: none.
Non-blocking nits (inline): the :ka batched potrf uses ka_potrf!'s 2-D ndrange, which allocates on the KA CPU backend only; getparam of per-member pivot statistics after a masked Cholesky refactorization returns stale totals for inactive members (worth a docstring sentence).
Closes #17
Task: T17 — opened by the workflow after the implementer run https://github.com/exanauts/SparseDirectSolver.jl/actions/runs/37063836067 ended without a pull request (the App token had expired). The Report block in TASKS.md and the commit message are the implementer's; the body of the PR template is not filled in.
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com