Repository navigation
Conversation
…ot dual (#314) Full-pivoting `lu_in_place` took the shared row-major copy from 64 * 64 elements up, ahead of its own transpose dual. On square matrices below 128 the dual runs within 1.05x of a column-major input (f64 64x64 one thread: 1.01x on Apple M5 Max, 1.03x on Zen 5) while the copy costs 1.24x / 1.32x, so those shapes now skip the copy (`row_major_takes_the_copy`). From 128 up the dual's row scaling walks a strided row every step (1024 one thread: 0.22 -> 3.26 ms for that stage alone, the update and pivot search equal), and on tall shapes the long swap turns strided and the trailing block wide (1.4-2.1x), so every other shape keeps the copy. Tests: `faer/tests/full_piv_lu_row_major_no_copy.rs` bounds the largest single allocation below m * n * 8 on 64/96/127 square and sees the copy on 128x128 and 256x32; the full-pivoting arm of the row-major adaptation test asks the new guard for bitwise versus rounding agreement and covers 127x127. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: runtime tests, the reported mutation run, or benchmark replay. At inspection, head Reviewed head: |
…dent tie order (#314) Review of #342. The square n < 128 exclusion from the row-major copy also applied to views with neither stride 1, e.g. 96x96 with strides (2, 192): those skipped the copy and fell to the separate matrix update and pivot search instead of the fused SIMD kernel. `row_major_takes_the_copy` now takes the strides and skips the copy only when the rows are contiguous (col_stride == 1); every other stride keeps the previous dispatch. The transpose dual breaks a magnitude tie by the smallest row first, the column-major search by the smallest column, so on ties the pivots and the transposition count differ by layout (zero 64x64 with A[(1,2)] = A[(0,4)], max_rank 1: (1,2) with two transpositions vs (0,4) with one). `lu_in_place` documents it under "Pivot ties" and a test pins the case; the adaptation test no longer says "the same pivots" unconditionally. Tests: the allocation test adds 96x96 with strides (2,192) and (192,2) (copy kept; fails at 96 bytes with the old guard) and a padded row-major view (100,1) (no copy). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d the clean-head run The worklog and PATCHSET said square 64..=127 passes on both machines; the timed sizes are 64, 80, 96, 112, 120 and 127 on Apple M5 Max and 64, 96 on Zen 5, the rest is interpolated. The earlier run measured b16d6b8 plus an uncommitted copy-bypass switch; the record now adds a paired run of the committed 32c615f source against b16d6b8 (f64/c64, 1T/4T, three repetitions: dual 0.89-1.02x a column-major input on the 24 changed cells) and names the switch run as not reproducible from a revision. The lu_in_place tie note no longer says the factors differ only on ties. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fineRun Tests run Reviewed head: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Not rerun: the full local suite, mutation checks, or benchmark timings. This review targets Reviewed head: |
…accumulators (#314) Second review of #342. The SIMD full-pivot search walks each column with four interleaved accumulators (three in the fused update) and merged them with a strict >, so an equal maximum held by the accumulator merged first won even with the larger row: zero 64x64 row-major with A[(2,6)] = A[(2,8)] = 2 and max_rank 1 pivoted on (2,8) on a two-lane build. best_score now also takes the other accumulator on an equal score with a smaller row. Inherited from upstream; it changes the tied pivot for every layout, to the order the scalar search and reduce_2d's comment already describe. The lu_in_place "Pivot ties" section described the copy exclusion as the whole condition. It now states the dispatch: the copy when row_major_takes_the_copy holds, else the input when |row_stride| < |col_stride| and its transpose otherwise (an 8x16 row-major matrix is never copied and picks (0,4), column-major (1,2)). Tests: pivot_search_keeps_the_smaller_row_across_accumulators (every row pair of a 64-row column, plain and fused search, three alignments; failed at rows (0,2) before the fix) and the integration tie test (8x16, 256x32 copied, and the within-column cases; returned (2,8,2) before the fix). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uite run Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Not checked: the full crate suite, fresh performance measurements, or completion of hosted CI. The complex pivot-contract mismatch was reproduced locally. Reviewed head: |
Closes the first slice of #314 (Plan item 2 only). Head:
809eaa2(code63790b0; the last commit is docs only; see "Measured code" below for which commit the timings cover) on314-flu-row-major-dual, basemain@b16d6b859ece9fe3b130757b02bc64281d82baa1.What this changes
full_pivoting::factor::lu_in_placeused to send every row-major input of at least 64·64 elements (tall or square) through the shared column-major copy (linalg::row_major_copy_pays), before its own transpose dual (lu_in_place_unblocked(.., transpose = true)) was ever tried. Now square matrices below 128 whose rows are contiguous (col_stride == 1; any row stride, so padded and reversed row-major views count) skip the copy and are factored in place through the dual. Every other shape, and every generic-stride view (neither stride 1, or column stride -1), keeps the copy as onmain.row_major_takes_the_copy(m, n, row_stride, col_stride): the old condition (|row_stride| != 1 && m >= n && n > 1 && row_major_copy_pays(m, n)) minuscol_stride == 1 && m == n && n < 128.row_major_takes_the_copyholds, otherwise on the input when|row_stride| < |col_stride|and on its transpose when not, and keeps the smallest column, then the smallest row, of that matrix. So a row-major input that is not copied breaks a tie by row first. The new square exclusion is not the only such case: wide row-major matrices and those below 64·64 elements were never copied (8×16 row-major withA[(1,2)] = A[(0,4)] = 2,max_rank = Some(1),Par::Seq: (0,4) with one transposition; column-major (1,2) with two). Documented onlu_in_place("Pivot ties") with that dispatch, as Row-major parity: run every dense factorization natively on row-major input via transpose duality (no whole-matrix copy), at column-major performance #314 asks.>, so the accumulator merged first won a tie even with the larger row: zero 64×64 row-major withA[(2,6)] = A[(2,8)] = 2pivoted on (2,8), column-majorA[(6,2)] = A[(8,2)]on (8,2), on Apple M5 (two f64 lanes).best_scorenow also takes the other accumulator on an equal score with a smaller row. The code is inherited from upstream; the fix changes the tied pivot for every layout, to the order the scalar search andreduce_2d's comment already describe. Not timed (one merge per column per step).row_major_copy_paysand the other families that share it (QR, column-pivoted QR, partial-pivot LU) are not touched.PATCHSET.md: the full-pivoting note in the "Row-major LU, partial pivoting" row. Its sentence "full pivoting is deliberately not adapted" had been out of date since10ee1c9.docs/worklogs/2026-11-09-full-piv-lu-row-major-dual.md. The full measurement comment: Row-major parity: run every dense factorization natively on row-major input via transpose duality (no whole-matrix copy), at column-major performance #314 (comment)Measurement 2: the committed code (Apple M5 Max,
32c615fvsb16d6b8)Measured code. This run measured
32c615f(clean committed tree,faer/srctree38ce727). The head63790b0changesfaer/srcafter that: the tie fix inbest_score(one more mask per accumulator merge, once per column per step) and doc comments. The head itself was not re-timed; that the tie fix leaves these ratios unchanged is not measured.32c615fgives A (column-major) and C (row-major through the dual); baseb16d6b8gives A and B (row-major through the copy). The ratio is taken within one process; the table gives each repetition and the median of three.01a1cb8with the one-line localset_threadspatch (as in measurement 1),--reps 5 --prime-ms 250 --statistic median, familyfull_piv_lu, rowfaer-fork, thein_placeline. f64 and c64,--threads 1/4.rustc,cargoabove 5% CPU, or othertarget/binary ran before and after it. 96 kept, 11 discarded and rerun (7 for rustc or cargo, 4 for other sessions' test binaries). 1-min load before a kept cell: 7.0–26.4. 18:41–19:01 JST, 2026-11-09. XProtect not recorded.fbench verifyon the head binary: all 64 cells per thread count ok (1T, 4T).Result. On the 24 changed cells (64–127), median C/A is 0.888–1.017; median B/A on
mainis 0.978–1.343. The 128 and 256 cells run the same code on both revisions (the copy) and are the control: C/A 0.976–1.181, B/A 1.005–1.150.Noise. Single repetitions swing far more than the medians: one control cell reads 0.650 and another 1.616, and the A arms of the two binaries in one repetition differ by up to 1.8× (last column). Three repetitions under this load are a scan, not a campaign comparison.
candidate_compare.pywas not run.gate log (load average 1 / 5 / 15 min before and after each cell)
Measurement 2 commands
Which sizes were timed. M5: 64, 80, 96, 112, 120, 127 (measurement 2, f64/c64, 1T/4T). Zen 5: 64 and 96 (the earlier #314 comment). The sizes in between were not timed; the guard takes them on the strength of their neighbours. 128 and above, and the tall shapes, come from measurement 1.
Measurement 1: boundary and large shapes (Apple M5 Max, f64,
b16d6b8+ uncommitted switch)Arms: A column-major input. B row-major input through the copy (
main). C row-major input through the dual (main+ a local, never committedFAER_FLU_COPY=offenvironment switch that skipped the copy branch on every shape). The switch is in no published revision, so arm C of this run cannot be rebuilt from a checkout; measurement 2 covers the committed code. Each value is the median of 15 samples (3 repetitions ×--reps 5).Absolute values (A / B / C, µs): 1T 64: 34.2 / 42.5 / 34.5; 1T 1024: 115,442 / 118,034 / 119,101; 4T 1024: 46,202 / 46,641 / 50,330. The full table, with min/median/max, is in the issue comment.
Why the boundary is square n < 128. The acceptance criterion is C within 1.05× of A.
Why non-square shapes are excluded. In the same ungated scan on M5, f64 C/A (1T / 4T) is:
Tall shapes are 1.40–2.06× (table). The band ends as soon as the matrix stops being square.
Where the dual loses (per-stage timers inside
lu_in_place_unblocked, local and not committed; run before the gate, at load about 5–7):A[(k, j)] *= inv, a strided O(n²) walk: 0.22 → 3.26 ms at 1024, 0.06 → 0.60 ms at 512.Measurement 1 conditions
ac68faa202026-05-25), releasetensor4all/faer-benchmark@01a1cb8fbench, familyfull_piv_lu, rowfaer-fork, thein_placeline. Vendor not linked, with the one-line localset_threadspatch (returns Ok forNAME == "none") also used in the Zen 5 commentb16d6b8+FAER_FLU_COPYswitch (uncommitted)--threads 1/--threads 4(faer::team::with_team; CSVrayon_threads= 1 / 4)on(B = order c, A = order f), thenoff(C = order c, A′ = order f). Cells are cycled per repetition, not grouped by armrustc,cargoabove 5% CPU, or othertarget// bench binary ran before and after it. 2 of 44 cells were discarded and rerun (one rustc, onestrided_basictest binary)XprotectServiceat 94% CPU, andXProtectRemediatorPirritat about 88% reported separately in that window. At 18:07 JST every XProtect process was at 0.0% CPU and noXProtectRemediator*process existed. Whether it ran during 17:50–18:01 is unknownload average before / after each cell (1 / 5 / 15 min)
Measurement 1 commands
Verification
Tests at
63790b0:Red → green for the tie fix: the two tie tests run with
best_scorereverted to the strict>(red), then with the fix (green), the sources touched before each build:faer/tests/full_piv_lu_row_major_no_copy.rs. A test-onlyGlobalAllocrecords the calling thread's largest single allocation duringlu_in_place.m·n·8.full_piv_lu_tie_order_depends_on_the_layoutpins the tie cases above: 64×64 and 8×16 in both layouts, 256×32 row-major (copied, so column first), and the two within-column ties.pivot_search_keeps_the_smaller_row_across_accumulators(unit test infactor.rs): every row pair of a 64-row column, through the plain search and the fused update at three alignments, so it holds whatever the lane count is.Par::Seqand on a 4-thread team.#[global_allocator](sparse::linalg::amd).linalg::tests::test_row_major_adaptation_matches_the_column_major_one. Its full-pivot arm now asks the new guard whether the two orders must agree bit for bit (copy) or up to rounding (dual). Pivots and transposition counts must still match; they do because its fixtures hit no magnitude tie, which the test comment now says instead of "the same pivots". 127x127 is added.Whole crate suite,
cargo test -p t4a-faer --no-fail-fast, on this host (Apple M5 Max), summed over the test binaries:63790b0(head)lq_allocates_nothing_60x300b16d6b8(base, run once on 2026-11-09 from a separate worktree and target dir)lq_allocates_nothing_60x300The head's failure reports allocations
[13, 12, 17, 17](expected all zero); the base log kept only the test names and totals, not the counts. It is one of the host allocation failures recorded indocs/worklogs/2026-11-04-gevd-repeated-complex-pairs.md; the hosted CI run passes it. The +3 at head are this PR's tests.failing / summary lines, head and base
Formatting: the repo is not
cargo fmt-clean under stable or nightly, so the changed lines were formatted by hand torustfmt.toml(max width 80, tabs) and the new test file with nightly rustfmt; no file outside the change was reformatted.Red check, with only the
!(m == n && n < 128)clause removed from the guard:Red check for the stride condition, with only
col_stride == 1 &&removed:The consistency test passes both with and without the clause, because both paths are correct; the red check belongs to the allocation test.
Not done: upstream faer was not measured. Padded and reversed row-major views (
(100, 1),(-100, 1)) are routed to the dual, which the allocation test shows for(100, 1)and the adaptation test covers for reversed rows (correctness); their speed was not measured, and the timed cells are contiguous row-major only. No tie test runs on a thread team; the parallel step merges column blocks with a strict>(earlier block, smaller column), which is read from the source, not tested. The dual kernel was not changed: a contiguous pivot-row scaling would plausibly close the square gap above 128, but that is not measured.🤖 Generated with Claude Code