Skip to content

fix(householder): gate the block apply's row split on the top block - #13

Merged
shinaoka merged 1 commit into
mainfrom
fix/issue12-triangular-assert
Oct 14, 2026
Merged

shinaoka merged 1 commit into
mainfrom
fix/issue12-triangular-assert

Conversation

@shinaoka

Copy link
Copy Markdown
Member

Fixes the assertion that blocked the campaign's householder_apply page
(tensor4all/faer#12).

Owner: the library, not the harness

The abort is reachable with no harness at all. qr_in_place on a 2048x2048
f64 at 8 threads with its own recommended_block_size (64) aborts, and so does
4096x4096 at 4 and 8 threads, before any householder_apply call runs:

$ qr_only 2048 2048 8 0        # t4a-faer at 9de95f7 (parent)
qr_in_place m=2048 n=2048 block=64 threads=8 order=C ...
Assertion failed at faer/src/linalg/matmul/triangular.rs:990:2
Assertion failed: lhs_ncols == rhs_nrows   - lhs_ncols = 64 - rhs_nrows = 16

The harness's own fixture is legal: it passes block_size = min(m, n) to
qr_in_place_scratch/qr_in_place, whose only contract on that argument is
> 0 and Q_coeff.ncols() == min(A.nrows(), A.ncols()). The campaign's 256x256
case is that shape, and the assertion fires inside qr_in_place -- the apply
the harness measures never runs.

Cause

The row split in apply_block_householder_on_the_left_in_place_generic (fork-only:
fa22a69, c061812, c8fea58) sums tmp = V^H C from partials over the
target's row chunks. The first n rows of V are its unit-lower triangular head
and their contribution is applied through that head's own n x n factor, so a
chunk whose rows are a strict subrange of [0, n) would need the head factor's
columns lo..top_hi -- a unit-upper trapezoid, a shape triangular::matmul
cannot state. The predicate asked only for depth, so any shape with
ceil(M / threads) < n handed the full n x n factor to a shorter row chunk.
The campaign's 256x256 QR reaches it through a panel apply with M = 256,
n = 64, K = 64 and 8 threads (chunk = 32).

Fix

One more conjunct, M.div_ceil(threads) >= n, so the split is taken only while
one chunk swallows the whole top block. No product is computed differently.
Every shape the split was measured on (a 4096-row target with a 16-wide basis,
chunk = 512; the recorded tests 2048x4 and 4096x16) has chunk >= n and is
unchanged. A column-sliced head factor would keep the split for narrower panels,
at the cost of two products and an explicit zero fill; rejected for now, recorded
in the work log.

Verification

  • Focused test
    threaded_panel_narrower_than_the_row_chunk_matches_the_sequential_one
    (256x128 basis, 64-column target, 4 threads, chunk = 64 < n = 128): on the
    parent it fails with lhs_ncols = 128, rhs_nrows = 64, with the gate it passes.
  • cargo test -p t4a-faer --lib: 204 passed, 0 failed, 2 ignored. No pre-existing
    failure. In particular sparse::linalg::qr::tests::test_numeric_simplicial_qr_1_transpose,
    which the issue names as failing on 22f23cf, passes on both 22f23cf and 9de95f7 --
    its calls are all Par::Seq, so it cannot reach the split; that second
    reproducer in the issue was misattributed.
  • The harness's verify over the whole declared householder grid (4, 8, 16, 32,
    64, 256, 1024x64, 4096x256; f64/c64; both orders; 1, 4 and 8 threads; fork and
    upstream rows): "all comparisons within tolerance", exit 0. That is the check
    record_run.py makes before publishing, so the population is recordable again.
  • Isolation: fbench built with scripts/build_for_faer_rev.sh --vendor mkl at
    this branch's commit against /tmp/upstream-faer (faer-v0.24.4); every build,
    test and measurement pinned to CPUs 4-11. No timing was claimed.

Work log: docs/worklogs/2026-10-14-row-split-top-block-gate.md, with the
PATCHSET.md row Row-split gate for the block Householder apply's top block.
The row split is fork-only, so nothing upstream was measured or reported.

The row split sums `tmp = V^H C` from partials over the target's row
chunks. The first `n` rows of `V` are its unit-lower triangular head and
their contribution is applied through the head's own `n x n` factor, so a
chunk whose rows are a strict subrange of `[0, n)` needs that factor's
columns `lo..top_hi` -- a unit-upper trapezoid no `triangular::matmul`
precondition can state. The predicate asked only for depth, so any shape
with `ceil(M / threads) < n` fed the full `n x n` factor against a
shorter row chunk and tripped `lhs_ncols == rhs_nrows` (64 against 32) in
`qr_in_place`, which is #12's abort.

One conjunct, `M.div_ceil(threads) >= n`, keeps the split exactly where
one chunk swallows the whole top block. Every shape the row split was
measured on has `chunk >= n` and is unchanged.
@shinaoka
shinaoka force-pushed the fix/issue12-triangular-assert branch from d3cc762 to 3497739 Compare October 14, 2026 03:58
@shinaoka
shinaoka merged commit 29be00a into main Oct 14, 2026
7 checks passed
shinaoka added a commit to tensor4all/faer-benchmark that referenced this pull request Oct 14, 2026
…e LU floor

The recorder materialises the pinned revision in the checkout it is handed and
records it as `library.commit`, so the pin *is* what a run measures and what names
the page. The first page is to record the fork's current `main` (`bfe4041`), which
carries tensor4all/faer#13 (the row-split gate for the block Householder apply),
#14 (the blocked LU's trailing-update floor) and the two records from #15 / #17,
so the pin moves before the run rather than with it.

No page exists for this campaign yet, so nothing becomes stale by this move.
shinaoka added a commit to tensor4all/faer-benchmark that referenced this pull request Oct 14, 2026
…e LU floor

The recorder materialises the pinned revision in the checkout it is handed and
records it as `library.commit`, so the pin *is* what a run measures and what names
the page. The first page is to record the fork's current `main` (`bfe4041`), which
carries tensor4all/faer#13 (the row-split gate for the block Householder apply),
#14 (the blocked LU's trailing-update floor) and the two records from #15 / #17,
so the pin moves before the run rather than with it.

No page exists for this campaign yet, so nothing becomes stale by this move.
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.

1 participant