Skip to content

basic: tile a spilled pair traversal (rank >= 9) as its compressed layout would be tiled - #429

Merged
shinaoka merged 10 commits into
mainfrom
fix/425-spilled-tiling
Nov 9, 2026
Merged

shinaoka merged 10 commits into
mainfrom
fix/425-spilled-tiling

Conversation

@shinaoka

@shinaoka shinaoka commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

Closes #425.

#422 cache-blocked plan and replay copies, but only for inline traversals (fused rank ≤ 8). A PairTraversal::Spilled traversal still walked untiled through apply_spilled_range, so a permuted copy whose fused rank stayed above 8 kept the #370 defect: one source cache line per element. This applies the invariant the issue asks for — an axis list is tiled exactly when the same layout given inline would be — out of line, leaving the inline types and walkers untouched.

What changed

piece file
the model's exits that need no metadata (zero element size, region within budget, every operand's strides nonzero and strictly increasing) as one predicate over an axis iterator, sharing the region arithmetic with the model itself strided-basic/src/block.rs (exits_before_metadata, region_of, axes_region)
the tiling decision over slices, shared by both storage shapes raw_ops.rs (tile_blocks)
PairTraversal::tiles — the single decision entry point a plan and a replay entry both call raw_ops.rs
spilled_pair_tiles: extent-1 axes dropped first in their given order, so a padded high-rank layout is decided and replayed by the 8-axis inline model raw_ops.rs
SpilledTiles + apply_spilled_tiled_box: the model's blocks beside rank-sized stride slices, and apply_tiled_box's tile odometer over them raw_ops.rs
replay_spilled_tiled: the view path's tree splitter, one box walk per leaf copy_plan.rs
replay_fused dispatch (Tiles::Inline from a spilled list goes through the inline walk unchanged) copy_plan.rs
slabs_share_no_line's fixed 8-entry tail → inline up to 8, heap above it (latent panic above 8) raw_ops.rs

CopyPlan/UpdatePlan replay and replay_given (hence the four replay_{copy,map}_raw[_uninit] entries, the erased plans, SlicePlan, ReversePlan, concatenate's per-segment copies and the initial operand copy of dynamic update / additive scatter) all reach the spilled route with the tile decision attached.

Order-sensitive destinations are untouched: replay_given still drops the tiles and limits the token to one slot for a destination it cannot prove injective, and both plan compilers reject one. The stateless copy_scale_raw/axpy_raw family (accumulating, no token) keeps its sequential spilled walk — out of scope here, as the issue lists it.

Allocation

Nothing is allocated before the decision. block::exits_before_metadata answers the model's exits from the axes as stored (a zero element size, a memory region within the 32 KiB budget, or every operand's strides nonzero and strictly increasing), using the same region_of/axes_region arithmetic as memory_region, u64 with the wide fallback; spilled_pair_tiles consults it before materializing anything.

So a spilled layout that one of those three pre-metadata exits keeps whole allocates exactly what it allocated before this PR:

  • CopyPlan/UpdatePlan execution: the rank-sized range cursor per execution (32 for 16 execute + 16 execute_scale calls on the [2;9] reversed-stride fixture, the same number as the base commit);
  • plan-free replay_*_raw: the axis list plus that cursor (2 per call, same as the base commit);
  • only a layout that the decision then tiles adds buffers, all rank-sized and created after the decision: the three model slices, the block vector, and the walk's extents and origins.

This is a statement about the three enumerated exits, not about "the model keeps it whole". A layout that needs the metadata to be declined — equal operand stride orders such as dims = [2; 9] with both stride arrays [1024; 9], whose modeled region (32768 B per operand) is over budget and whose strides are not strictly increasing — pays for the three rank-sized vectors before the model declines it. That path allocates the same rank-sized metadata it allocated before this PR does not change, but it does allocate, so it is not covered by the sentence above.

For the [2;9] reversed-stride fixture the arithmetic is: per operand, the sub-cache-line strides contribute (2-1)·(8+16+32) = 56 bytes → 1 contiguous line, and the 6 axes of extent 2 at ≥ 64-byte strides → 2^6 line-blocks of 64 B = 4096 B per operand, 8192 B of the 32768 B budget, so the model keeps the full dims and the decision never allocates. That is what alloc_matrix's restored <= 2 and copy_plan_alloc's restored 32 now assert.

Measurement — the rank-9 cliff is gone, and the issue's no-cliff bar is not met

Harness (a b425 binary with a path dependency on the reviewed worktree, --release, nothing else):

use std::time::Instant;
use strided_basic::{replay_copy_raw, Exec, RawStridedMut, RawStridedRef};

fn permuted(dims: &[usize]) -> (Vec<isize>, Vec<isize>) {
    let mut s = 1isize;
    let dst: Vec<isize> = dims.iter().map(|&d| { let v = s; s *= d as isize; v }).collect();
    let mut s = 1isize;
    let mut src = vec![0isize; dims.len()];
    for a in (0..dims.len()).rev() { src[a] = s; s *= dims[a] as isize; }
    (dst, src)
}

fn bench(label: &str, dims: &[usize], reps: usize) -> f64 {
    let (dst_s, src_s) = permuted(dims);
    let total: usize = dims.iter().product();
    let src: Vec<f64> = (0..total).map(|k| (k % 97) as f64).collect();
    let mut out = vec![0.0f64; total];
    let mut best = f64::INFINITY;
    let mut sum = 0.0;
    for _ in 0..reps {
        let src_ref = RawStridedRef::new(&src, dims, &src_s, 0).unwrap();
        let mut dst = RawStridedMut::new(&mut out, dims, &dst_s, 0).unwrap();
        let t = Instant::now();
        replay_copy_raw(&mut dst, &src_ref, Exec::serial()).unwrap();
        let ns = t.elapsed().as_nanos() as f64 / total as f64;
        best = best.min(ns);
        sum += ns;
    }
    println!(
        "{label:<34} min={best:8.3} ns/elem  mean={:8.3} ns/elem",
        sum / reps as f64
    );
    best
}

fn main() {
    let rank8 = bench("permuted rank-8 [4,16,16,8,8,8,8,4]", &[4, 16, 16, 8, 8, 8, 8, 4], 25);
    let rank9 = bench("permuted rank-9 [4,4,8,8,8,8,8,8,4]", &[4, 4, 8, 8, 8, 8, 8, 8, 4], 25);
    let padded = bench("permuted rank-9 + inert [4,16,16,8,8,8,8,1,4]", &[4, 16, 16, 8, 8, 8, 8, 1, 4], 25);
    let rank12 = bench("permuted rank-12 [4;12]", &[4; 12], 25);
    println!(
        "ratios vs rank-8: rank9/rank8 = {:.2}x, rank9padded/rank8 = {:.2}x, rank12/rank8 = {:.2}x",
        rank9 / rank8,
        padded / rank8,
        rank12 / rank8
    );
}

Raw sample, base commit b2358fb (before), taskset -c 0, host AMD Ryzen AI MAX+ 395 (EVO-X2), 32 CPUs, 2 × 64 MiB L3 (0-7,16-23 / 8-15,24-31), load average 0.57:

permuted rank-8 [4,16,16,8,8,8,8,4]  min=   3.159 ns/elem  mean=   3.265 ns/elem
permuted rank-9 [4,4,8,8,8,8,8,8,4]  min=  21.430 ns/elem  mean=  21.523 ns/elem
permuted rank-9 + inert [..,1,..]    min=  21.192 ns/elem  mean=  21.288 ns/elem
permuted rank-12 [4;12]              min=  21.416 ns/elem  mean=  21.508 ns/elem

Raw sample, this branch (taskset -c 0, four consecutive runs, load average 0.78 → 1.66). One run in full:

# serial replay_copy_raw, best of 25; ns per element
permuted rank-8 [4,16,16,8,8,8,8,4]  min=   3.109 ns/elem  mean=   3.196 ns/elem
permuted rank-9 [4,4,8,8,8,8,8,8,4]  min=   3.438 ns/elem  mean=   3.520 ns/elem
permuted rank-9 + inert [..,1,..]    min=   3.108 ns/elem  mean=   3.180 ns/elem
permuted rank-12 [4;12]              min=   4.027 ns/elem  mean=   4.118 ns/elem

ratios vs rank-8: rank9/rank8 = 1.11x, rank9padded/rank8 = 1.00x, rank12/rank8 = 1.30x

The other three runs as printed, ratio line only, so the spread is visible rather than one cherry-picked sample:

ratios vs rank-8: rank9/rank8 = 1.10x, rank9padded/rank8 = 1.00x, rank12/rank8 = 1.29x
ratios vs rank-8: rank9/rank8 = 1.07x, rank9padded/rank8 = 1.00x, rank12/rank8 = 1.26x
ratios vs rank-8: rank9/rank8 = 1.06x, rank9padded/rank8 = 1.00x, rank12/rank8 = 1.27x

So, against the issue's proposed acceptance:

  • met: the padded rank-9 layout is the rank-8 layout in slots, decision and walk (1.00×, three runs), which was the second defect the issue named;
  • borderline: rank 9 is 1.06–1.11× of rank 8 (bar 1.10×), i.e. it straddles the bar across runs rather than clearing it;
  • not met: rank 12 is 1.26–1.30× of rank 8 against the same 1.10× bar. The model returns the same block shape for [4;9] and [4;12] ([4,4,4,1,1,1,2,4,4] vs [4,4,4,1,1,1,1,1,1,2,4,4], 2048 elements each), but the tile counts differ: prod(ceil(dim / block)) gives 128 tiles for rank 9 and 8192 for rank 12 (an earlier revision of this body said "128 tiles each", which was wrong). The tile density per element is the same for both (128 / 4^9 = 8192 / 4^12 = 1/2048), so the counts by themselves do not explain an ns-per-element difference: they establish only that the block choice is identical for those two shapes. Two further limits on what these numbers say: the timed rank-9 input is [4,4,8,8,8,8,8,8,4], not [4;9], so the block vector quoted above is the model output for a shape that was not timed; and the harness times replay_copy_raw, which includes tile selection, so it cannot separate selection from walking. Recording the selected blocks for the timed shapes and timing selection apart from the walk is the follow-up in the issue, and this PR does not pretend to have closed it.

Not measured here: 4-thread / team throughput, the Julia @strided ratio, the small-execute instruction-count guard, the untiled-spilled <=5% guard, and inline-path codegen stability. The inline path is unchanged at source (it now passes through Option<Tiles> and the extracted decision code), but source similarity is not a measurement: those two guards are named follow-ups, not results of this PR. The numbers above are one serial thread on one machine, an unpinned observing shell, and a host whose load average moved between 0.57 and 1.66 across the samples, so treat the ratios as indicative and the 21.4 → 3.4/4.0 ns per element collapse as the signal.

Verification

Red first, with the whole spilled route removed (spilled_pair_tiles returning None, everything else unchanged):

$ cargo test -p strided-basic --lib -- raw_ops::tiled_tests
test raw_ops::tiled_tests::spilled_plan_entries_visit_tiles ... FAILED
test raw_ops::tiled_tests::spilled_layouts_tile_as_their_compressed_rank_would ... FAILED
test raw_ops::tiled_tests::tiled_replays_equal_the_column_major_oracle ... FAILED

thread 'main' panicked at strided-basic/src/raw_ops/tests/tiled_tests.rs:460:9:
CopyPlan::execute: the spilled plan must visit cache tiles, got 0 boxes
thread 'main' panicked at strided-basic/src/raw_ops/tests/tiled_tests.rs:338:5:
the rank-9 copy must visit cache tiles, got 0 boxes
thread 'main' panicked at strided-basic/src/raw_ops/tests/tiled_tests.rs:504:9:
4^9 reversed must take the tiled walk

test result: FAILED. 9 passed; 3 failed; 0 ignored; 0 measured; 235 filtered out

Red second, for the prepared-plan entries specifically: suppress tiles only for a spilled traversal in the two plan replay bodies, leaving the plan-free entries alone.

$ cargo test -p strided-basic --lib -- raw_ops::tiled_tests::spilled_plan_entries_visit_tiles
thread 'main' panicked at strided-basic/src/raw_ops/tests/tiled_tests.rs:460:9:
CopyPlan::execute: the spilled plan must visit cache tiles, got 0 boxes
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 246 filtered out

Red third, for the early exit: evaluate the region in u64 only, as if an overflow meant "does not fit".

$ cargo test -p strided-basic --test alloc_matrix
thread 'main' panicked at strided-basic/tests/alloc_matrix.rs:1022:5:
a cache-fit spilled replay allocated 5 times
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

All three mutations are reverted. Green, at this head:

$ cargo test -p strided-basic --lib -- raw_ops::tiled_tests
test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 235 filtered out

$ cargo test -p strided-basic --lib
test result: ok. 249 passed; 0 failed

$ cargo test --workspace
test result: passed=1140 failed=0

$ cargo test --workspace --features strided-basic/parallel,strided-kernel/parallel
test result: passed=1178 failed=0

$ cargo test -p strided-basic --doc
test result: ok. 92 passed; 0 failed

$ cargo fmt --all -- --check                       # clean
$ cargo clippy --workspace --all-targets           # 0 warnings, 0 errors
$ cargo doc -p strided-basic --no-deps             # 0 warnings

Coverage of the new route: layouts() gained 4^9 reversed, a rank-12 reversed layout, and a rank-9 layout with a negative source axis and a cropped base, so every typed entry (copy, map, scale, conj, update with beta ≠ 0, and the _uninit rows, f64 and Complex64, serial and team) runs against the independent per-element oracle through the spilled tile walk. spilled_plan_entries_visit_tiles pins each prepared-plan entry to an executed tile count, spilled_layouts_tile_as_their_compressed_rank_would does the same for the plan-free entries and the padded layout and, under --features parallel, checks that the spilled split leaves partition the box exactly once per slot. The two allocation cases pin the cache-fit exit (including the overflowing-stride variant) to the pre-#425 counts.

Review

Independent reviews of the finished diff (gpt-6.1-sol, read-only, reading the source rather than restating the description), plus the maintainer's own review commits on this branch.

Round 1, on the first head: 0 BLOCKER, 0 IMPORTANT, 1 MINOR — replay_map_raw_uninit was missing from the generic entry/type matrix — fixed by adding that row to check_entries, so it now runs for f64 and Complex64 on serial and team tokens. Its delete-list item, a redundant rank < 2 guard in spilled_pair_tiles (pair_tiles already declines fewer than two moving axes), is deleted.

Round 2 findings, and what closed each one:

  • the spilled decision materialized its three rank-sized slices (compressed dims, destination and source strides) before asking the model, so a layout the model's cache-fit exit declines paid three allocations for a decision that ended in "no tiles", and the regression test had been relaxed 32 → 128 instead of the code being fixed. Closed by deciding the exits from the axes as stored (block::exits_before_metadata over an axis iterator, with region_of/axes_region sharing the model's own region arithmetic) and allocating only after. copy_plan_alloc is back at 32 and alloc_matrix's rank-9 bound is back at 2, both the pre-Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals) #425 numbers.
  • the routing regression only queried layouts. spilled_plan_entries_visit_tiles now pins an executed tile count for CopyPlan::execute, execute_scale, execute_conj, UpdatePlan::execute, and both _uninit variants; spilled_layouts_tile_as_their_compressed_rank_would does the same for the plan-free replay entries, the padded layout, and (added in review) a rank-9 layout with a negative source axis and a cropped base.
  • a u64 region overflow was read as "does not fit", which re-opens the allocation hole for a zero-sized operand carrying a stride no allocation could hold. Closed by the wide (u128) fallback in axes_region, and pinned here by check_overflowing_stride_keeps_the_exit_allocation_free.

No correctness BLOCKER or IMPORTANT is open. The "not verifiable by reading" items are listed under Scope: nothing in the reviews measured 4T, Julia, instruction counts or codegen.

Not done here

  • the view path's rank ≥ 9 kernel, dynamic windows above fused rank 8, the tokenless raw family, concatenate's whole-plan parallel path and per-operand element size in the shared model stay out, as the issue scopes them;
  • the stateless copy_scale_raw/axpy_raw family keeps its sequential spilled walk;
  • the rank-12 gap above and the 4T / Julia / instruction-count guards are open, to be handled in the issue.

…layout

The cache-block model reached only the inline traversal, so a permuted copy
whose fused rank stayed above 8 walked untiled through apply_spilled_range
and kept the #370 defect: one source cache line per element.

One invariant now: an axis list is tiled exactly when the same layout given
inline would be. The decision core is one function over dims/dst/src slices,
and the extent-1 axes are dropped first, so a padded rank-9 layout is decided
by the 8-axis model it compresses to and replayed through the inline tile
walk unchanged. Above that, SpilledTiles keeps the model's blocks beside
rank-sized stride slices, apply_spilled_tiled_box runs the same tile odometer
as apply_tiled_box over them, and replay_spilled_tiled splits the box with
the view path's tree splitter. slabs_share_no_line no longer has a fixed
8-entry tail, which would have panicked above that rank (issue #425).

f64 serial, 1 core, 2^24 elements, ns/element (min of 15):
  rank 8 [4,16,16,8,8,8,8,4]  3.10 -> 3.10
  rank 9 [4,4,8,8,8,8,8,8,4] 21.43 -> 3.42
  rank 9 + inert axis        21.19 -> 3.10
  rank 12 [4;12]             21.42 -> 4.01

Red before the fix: '4^9 reversed must take the tiled walk' and 'the rank-9
copy must visit cache tiles, got 0 boxes'.
@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

Repository rules review

Repository rules review (b2358fbf470e076379b6ac855d17d083e7f5fb40...d521b0fb84374aa7830891e5c28e392b3a557e8a)
Verdict: pass
Findings:
- [warn] llm-skipped (External LLM Review) <unknown>: External LLM review was skipped
  External LLM review is permanently disabled; deterministic checks only.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 2
UNVERIFIED: 3

Critical (must not merge)

none

Needs changes

  • Early-exit layouts acquire three unnecessary allocations per execution — strided-basic/src/raw_ops.rs:466–475 allocates the compressed dimensions and both stride vectors before block.rs:97 can return the cache-fit decision, violating Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals) #425’s explicit requirement that existing early-exit cases allocate nothing new. Reproduce with the existing [2; 9] reversed-stride fixture in strided-kernel/tests/copy_plan_alloc.rs:194–225: each operand’s modeled region is 4,096 bytes, so their combined 8,192 bytes fit the 32 KiB budget, yet every prepared execution now allocates these three buffers unnecessarily. The changed assertion accepts 128 allocations instead of 32 across 32 calls—16 iterations containing two executions each. Preserve the allocation-free decision exits instead of weakening this regression assertion.

  • The routing regression does not protect either prepared-plan entry — strided-basic/src/raw_ops/tests/tiled_tests.rs:217–218,256–257 checks numerical results, while the new executed-tile assertions at :330 and :372 observe only replay_copy_raw; the selection assertion at :409–411 queries the layout independently of execution. A targeted mutation that suppresses tiles only for PairTraversal::Spilled at strided-basic/src/copy_plan.rs:319 or :530 restores the original untiled-plan defect without changing those numerical results or the raw-copy routing assertions. Add per-entry executed-tile assertions for CopyPlan and UpdatePlan, including their uninitialized variants, then verify that this targeted mutation fails cargo test -p strided-basic --lib -- raw_ops::tiled_tests; that mutation experiment was not executed during this review.

Unverified claims

  • Performance acceptance is not established — the PR’s Measurement table concerns the new walker at strided-basic/src/raw_ops.rs:666–729, but the supplied diff contains neither its benchmark harness nor raw samples. The reported numbers themselves give 4.02 / 3.13 = 1.284× for rank 12, exceeding Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals) #425’s proposed 1.10× limit; Julia comparisons and the 4T and instruction-count guards are explicitly unmeasured. Reproduction requires the exact harness and baseline/candidate results, not just the table; the claimed attribution of the remaining cost to loop depth is also unverified without profiling.

  • The pasted test totals and red/green experiment are not independently established — the Verification claims cover strided-basic/src/raw_ops/tests/tiled_tests.rs:310–428, but complete local logs were not supplied and the commands were not rerun here. GitHub CI run 42584663509 was verified to target a87776db20c3e77078876e011b6f66facb3be494, but remained in_progress with no conclusion at inspection; completed lint jobs do not establish completion of the test and parallel lanes.

  • The cited independent-review verdict cannot be checked — the PR asserts that /tmp/review425/post425.out covers the finished source changes, including strided-basic/src/copy_plan.rs:319,530; cannot read that report because the path does not exist on this host. Its reviewed SHA, scope and disposition of findings therefore remain unverified.

Checked and fine

none

Not run: Cargo tests, targeted mutations or benchmarks; kache doctor --verify timed out before verification completed. No candidate source files were modified.


Reviewed head: a87776db20c3e77078876e011b6f66facb3be494 at 2026-11-09T08:16:15Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 2
UNVERIFIED: 2

Critical (must not merge)

none

Needs changes

  • Early-declined spilled copies now allocate three unnecessary buffers — strided-basic/src/raw_ops.rs:466–474. The compressed dimensions and both stride vectors are allocated before the model can take its allocation-free early exits, violating Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals) #425’s stated requirement that those exits—including the existing rank-9 allocation pins—add no allocations. Reproduction: restore the old first <= 2 assertion at strided-basic/tests/alloc_matrix.rs:984, then run cargo test -j 4 -p strided-basic --test alloc_matrix; I reproduced the failure, “rank-9 replay allocated 5 times.” Preserve the model’s early exits before allocating these slices instead of weakening the allocation guard.

  • The regression tests stay green when spilled prepared-plan tiling is disabled — strided-basic/src/raw_ops/tests/tiled_tests.rs:226–279,405–423, covering the dispatch changes at strided-basic/src/copy_plan.rs:325,534. Reproduction: retain tile selection, but replace tiles.as_ref() at both prepared-plan replay calls with tiles.as_ref().filter(|_| self.fused.inline().is_some()), leaving raw replay unchanged; then run cargo test -j 4 -p strided-basic --lib --features parallel -- raw_ops::tiled_tests. I reproduced 11 passed, 0 failed despite disabling the fix for spilled CopyPlan and UpdatePlan executions. The oracle checks numerical results, while the routing counters only protect raw replay; add independent routing assertions for the prepared-plan entries.

Unverified claims

  • Performance acceptance and the explanation for the remaining slowdown are not established — strided-basic/src/raw_ops.rs:688–725, PR Measurement section. The reported rank-12 ratio is 4.02 / 3.13 = 1.2843, exceeding Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals) #425’s proposed 1.10× limit; the timings alone do not establish that deeper loop nesting causes the residual cost. I cannot read an accompanying benchmark harness or raw measurement artifact from this diff. Reproduction requires the stated rank-8/9/12 layouts under the required observer isolation, plus the omitted Julia, 4T, instruction-count and inline-codegen checks; green correctness CI does not establish these guarantees.

  • The quoted historical independent-review result cannot be audited — strided-basic/src/raw_ops/tests/tiled_tests.rs:262–264, PR Review section. The added generic uninitialized-map row is visible, but I cannot read /tmp/review425/post425.out: that path does not exist on this host. Reproduction: inspect the referenced report to verify its verdict and complete finding list; the diff cannot establish the quoted 0 BLOCKER / 0 IMPORTANT / 1 MINOR result.

Checked and fine

  • CI run 42584663509 completed successfully for head a87776d; inspected jobs matched that SHA.
  • Temporary mutations were restored; affected source/test files match the downloaded head archive.
  • Not rerun: full workspace or benchmarks. Initial kache verification found no corruption; repair verification timed out.

Reviewed head: a87776db20c3e77078876e011b6f66facb3be494 at 2026-11-09T08:19:05Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 2
UNVERIFIED: 2

Critical (must not merge)

none

Needs changes

  • The submitted head fails the formatting gate. strided-basic/src/block.rs:437-438 and strided-basic/src/raw_ops/tests/tiled_tests.rs:442-452: at dec05ef15d09d5c69714030f414e4e82592b2c6b, cargo fmt --check exits with status 1 on these two hunks. This is the actual failure in CI job 129056862445, not a stale run or infrastructure diagnosis. Reproduce by checking out that SHA and running cargo fmt --all -- --check; the PR body's earlier “clean” result at a87776d does not cover these additions.

  • The new unsafe spilled walker has no negative-stride oracle case. strided-basic/src/raw_ops/tests/tiled_tests.rs:135-144: the negative/cropped case has rank 3 and therefore exercises the inline walker; both added spilled cases use positive strides and zero initial bases. A mutation changing src: tiles.src[axis] to src: tiles.src[axis].abs() at strided-basic/src/raw_ops.rs:695 is indistinguishable on those spilled cases, despite breaking negative-source traversal. Add a concrete row using dimensions [4; 9], destination strides 4^axis, source strides 4^(8-axis) with the last source stride negated, and source base 8 with backing length 4^9 + 5; assert Tiles::Spilled, actual tile visits, and oracle equality for serial/team and initialized/uninitialized entries. This also supplies the missing cropped-base case for the separate walker requested by Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals) #425. The mutation was not executed here.

Unverified claims

  • The performance evidence does not establish the proposed no-cliff acceptance or unchanged fast-path overhead. strided-basic/src/raw_ops.rs:675-735 and PR body, “Measurement”: the timings concern a87776d, before the follow-up decision changes, and no reproducible harness or raw measurement artifact is supplied in the diff. Even accepting the reported numbers, rank 12 is 4.02 / 3.13 = 1.284×, exceeding Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals) #425's proposed 1.10× bar. The explanation that this comes specifically from the deeper loop nest is also unverified without profiling or instruction-count evidence. Reproduce the acceptance check with the listed rank-8/rank-9/rank-12 layouts at the submitted head, including the requested 1T/4T, Julia, untiled-spill and instruction-count guards; the body explicitly says those guards were not measured.

  • The independent-review verdict and mutation-test transcripts cannot be audited from the supplied artifacts. strided-basic/src/block.rs:430-443, strided-basic/src/raw_ops/tests/tiled_tests.rs:409-460, and PR body, “Review” / “Review follow-up”: cannot read /tmp/review425/post425.out on this host, and the diff contains neither that report nor the red-run artifacts. The cited original review does not establish coverage of the subsequently added early-exit implementation. The successful repository-rules gate is not substitute evidence: the head's separate LLM-review job is marked skipped. Attach the report with its reviewed SHA and the exact mutation patches/logs to make these claims auditable.

Checked and fine

none

Not checked locally: Cargo tests, mutation runs, or benchmarks; runtime correctness and performance beyond the inspected evidence remain unverified.


Reviewed head: dec05ef15d09d5c69714030f414e4e82592b2c6b at 2026-11-09T08:36:37Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 2
UNVERIFIED: 2

Critical (must not merge)

none

Needs changes

  • The submitted head fails formatting CI — strided-basic/src/raw_ops/tests/tiled_tests.rs:144,394,409. The rustfmt job for 5d7f4b4 exits with code 1 on these added expressions; the body’s clean formatting result concerns an earlier revision. Reproduce with cargo fmt --check using default rustfmt settings. The failing job log contains all three formatting diffs.

  • The replacement for the above-eight-entry tail panic has no targeted regression test — strided-basic/src/raw_ops.rs:555–566; strided-basic/src/raw_ops/tests/tiled_tests.rs:413. The added “model declines” fixture is rank 2, and the added rank-9/rank-12 reversed fixtures produce tails of only 6 or 7 entries for f64/Complex64, so they do not exercise the new heap arm. Add a selection test with:

    dims = [8,256,2,2,2,2,2,2,2,2]
    dst  = [1,8,2048,4096,8192,16384,32768,65536,131072,262144]
    src  = [1,2048,1024,512,256,128,64,32,16,8]
    

    Running the actual Rust block model on this fixture gives blocks [8,16,1,1,1,1,2,2,2,2], hence first_cut = 1 and a nine-entry tail; both operands have disjoint 64-byte slabs, so selection should return None. Assert that result through PairTraversal::given(...).tiles::<f64,f64>(), and verify that replacing the heap arm with a panic fails the test.

Unverified claims

  • Performance acceptance and the explanation for the remaining rank-12 cost are unverified — strided-basic/src/raw_ops.rs:447–492,692–715. The supplied diff contains no executable harness or raw samples for the timing table. Its reported numbers give 4.02 / 3.13 = 1.284×, exceeding the issue’s proposed 1.10× rank-12 bound; they do not establish that the residual is caused by the loop nest rather than selection or other overhead. Reproduce on the final head with the stated layouts, separating selection from walking before attributing the cost. Julia, 4T, instruction-count and untiled-spill regression guards remain unmeasured.

  • The quoted independent-review verdict cannot be audited — strided-basic/src/block.rs:440–459; .github/workflows/review_bot.yml:80–89. I cannot read /tmp/review425/post425.out, so its reviewed SHA, scope and verdict are unverified, including coverage of the later allocation-exit changes. The green repository-rules job runs --dry-run --llm-skipped-reason, so it does not supply that missing independent review. Attach the report with its reviewed SHA and identify which subsequent source changes it covers.

Checked and fine

cargo test -j 4 -p strided-basic --lib --features parallel -- raw_ops::tiled_tests: 12 passed, 0 failed.
kache doctor --repair --verify: 64,780 valid entries, 0 corrupted; stale GCC shims remain reported.
Not checked locally: the full workspace matrix, performance measurements, or the unavailable review report.


Reviewed head: 5d7f4b4908038e5cfaa3893e428a2948e056caa5 at 2026-11-09T08:45:00Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

…ating nothing

The cache-block model's exits are answered from the axes before any rank-sized
slice is materialized (#425). A zero-sized operand can carry a stride whose
byte magnitude overflows the checked u64 region arithmetic while the region
itself fits the budget, so the exit must use the wide fallback: reading the
overflow as "does not fit" would allocate the three model slices for a
decision that ends in "no tiles".

Pins that case, and restores the existing rank-9 replay bound to the pre-#425
two allocations for the cache-fit fixture ([2;9] reversed strides: 4096 B per
operand, 8192 B of 32768 B).

Red with the u64-only region:
  a cache-fit spilled replay allocated 5 times
Green after:
  test result: ok. 1 passed; 0 failed
@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 4
UNVERIFIED: 1

Critical (must not merge)

none

Needs changes

  • The overflowing-stride fixture violates its constructor’s documented contract — strided-basic/tests/alloc_matrix.rs:1003-1009 constructs a view over [(); 1] whose reachable element offsets include 1 << 61, although RawStridedRef::new_unchecked requires every reachable index to lie inside data (strided-view/src/raw.rs:935-936), without a ZST exception. Reproduce by replacing this construction with RawStridedRef::new on the same arguments: bounds validation rejects it. Construct a sufficiently long logical ZST slice and use the checked constructor; the current fixture does not establish the allocation guarantee for an in-contract input. This is a contract violation, not a demonstrated memory-access failure in the current walker.

  • The new unsafe walker lacks a clipped-edge regression case — strided-basic/src/raw_ops/tests/tiled_tests.rs:139-153 adds spilled fixtures whose model blocks divide their full extents; the explicit clipped-tile assertion at :542-545 still exercises only the inline walker. Consequently, these serial spilled fixtures do not test the partial-final-tile calculation at strided-basic/src/raw_ops.rs:708. Add a reversed rank-9 layout with dimensions [4,4,5,4,4,4,4,4,4]: the f64 model produces blocks [4,4,3,1,1,1,2,4,4], requiring the second tile along axis 2 to have extent 2 rather than 3. Check this through the oracle for initialized and uninitialized replay, and assert that the fixture actually contains a remainder.

  • The “128 tiles each” calculation is wrong — the tile grid implemented at strided-basic/src/raw_ops.rs:704-733 contains 8,192, not 128, tiles for the PR’s stated rank-12 dimensions and block vector. Reproduce with product(ceil(dim / block)): [4;9] with [4,4,4,1,1,1,2,4,4] gives 128; [4;12] with [4,4,4,1,1,1,1,1,1,2,4,4] gives 8,192. Both tiles contain 2,048 elements, but the total tile counts differ by 64×. Correct the measurement explanation; that arithmetic does not establish the asserted cause of the residual gap.

  • The allocation claim incorrectly includes layouts that the model keeps whole after building metadata — strided-basic/src/raw_ops.rs:478-486 materializes three heap-backed vectors before tile_blocks can decline at :421. A concrete counterexample is dims = [2;9], both stride arrays [1024;9], separate f64 buffers of length 9,217, offset zero, and serial replay_copy_raw. Its estimated region is 65,536 bytes and its strides are not strictly increasing, so it bypasses the allocation-free exits; the model then keeps every dimension, and tile_blocks returns None after those vectors were created. Count allocations around that call using the existing allocation harness. Narrow the PR body’s “model keeps whole” and “only a layout the model actually tiles adds buffers” statements to the specific pre-metadata exits.

Unverified claims

  • Performance measurements and non-regression guarantees — strided-basic/src/raw_ops.rs:447 and strided-basic/src/copy_plan.rs:918 introduce the measured selection and replay paths, but the supplied benchmark program prints neither the reported means nor the ratio lines presented as raw output. Running the pasted main cannot reproduce that output format; cannot read the actual measurement harness or original run artifacts from the supplied material. The timing ratios, residual-gap attribution, inline-path performance/codegen stability, untiled-spilled regression bound, and 4T/Julia comparisons therefore remain unverified here. The PR already reports the rank-12 result above its proposed 1.10× bar.

Checked and fine

  • CI run 42590558162 completed successfully for exact head af314138250f4fa91c8ff423a8cd76e63f85e2f7; its parallel-job log records the new spilled tests executing.
  • No local Cargo, mutation, or benchmark reruns were performed; the secondary review did not produce a completed report and was not counted as evidence.

Reviewed head: af314138250f4fa91c8ff423a8cd76e63f85e2f7 at 2026-11-09T09:08:08Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 2
UNVERIFIED: 2

Critical (must not merge)

none

Needs changes

  • The spilled oracle fixtures do not detect broken last-tile clipping — strided-basic/src/raw_ops/tests/tiled_tests.rs:141–153, exercising strided-basic/src/raw_ops.rs:708. In a temporary copy, I replaced tiles.blocks[axis].min(box_dims[axis] - origin[axis]) with tiles.blocks[axis].min(box_dims[axis]); the existing tiled_replays_equal_the_column_major_oracle test still passed under --no-default-features --features parallel. This mutation incorrectly gives a trailing tile the full block extent when the remaining extent is smaller, allowing duplicate or out-of-box accesses. Add an odd-sized spilled fixture, such as reversed(&[5; 9]), assert that a selected block does not divide its dimension, and compare initialized and uninitialized replays against the oracle. The existing clipped rank-2 fixture exercises the other walker.

  • The PR’s “128 tiles each” calculation is wrong — strided-basic/src/raw_ops.rs:707–710,725, PR body, “Measurement.” Reproduce directly from the two quoted block vectors using product(ceil(dim / block)): [4;9] requires 128 tiles, whereas [4;12] requires 8192 tiles, not 128; both quoted blocks contain 2048 elements. Correct this calculation before using it to attribute the remaining slowdown exclusively to deeper loop nests. This arithmetic does not establish a performance regression, but the stated diagnostic evidence is false.

Unverified claims

  • The published timing output cannot be reproduced by the supplied harness — strided-basic/src/raw_ops.rs:725 and strided-basic/src/copy_plan.rs:920, corresponding to the measured spilled walk. The harness prints one best-of-25 value per layout; it computes neither the reported means nor the ratio lines. Running the supplied program therefore cannot produce the quoted “raw sample.” The actual measurement harness and raw result artifacts are absent from the supplied diff; cannot read them. Supply the program that produced those outputs, including the aggregation procedure.

  • Inline and untiled-spilled performance stability remains unverified — strided-basic/src/raw_ops.rs:131–135,409 and strided-basic/src/copy_plan.rs:319. Inline execution now passes through Option<Tiles> and extracted decision code, so source-level similarity does not establish unchanged instructions or code generation. The supplied measurements cover tiled permutations, not the issue’s small-execute instruction-count guard or untiled-spilled ≤5% guard. Reproduce those guards against b2358fb at the submitted SHA; the green correctness lanes do not establish them.

Checked and fine

CI run 42590558162 targets exactly af314138250f4fa91c8ff423a8cd76e63f85e2f7; the default and feature test steps executed rather than being skipped.

Not checked: full local workspace and default-feature tests; the initial local build timed out. The unmodified parallel/no-SIMD tiled suite passed 13 tests; the clipping mutation’s oracle test also passed.


Reviewed head: af314138250f4fa91c8ff423a8cd76e63f85e2f7 at 2026-11-09T09:19:43Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 1
UNVERIFIED: 3

Critical (must not merge)

none

Needs changes

  • The overflow regression fixture violates the constructor’s documented safety contract — strided-basic/tests/alloc_matrix.rs:1005–1009 supplies a one-element slice to RawStridedRef::new_unchecked, although coordinate [1,0,0,0,0,0,0,0,0] reaches element index 1 << 61; strided-view/src/raw.rs:933–936 requires every reachable index to lie inside data, without a zero-sized-type exception. Reproduce by replacing this constructor with RawStridedRef::new(...).unwrap(): bounds validation rejects the fixture because (1 << 61) >= unit.len(). Zero byte displacement does not establish the documented element-index precondition. Use a logically long ZST slice, such as vec![(); (1usize << 61) + 1], and the checked constructor; this needs no element-storage allocation and preserves the byte-stride overflow being tested.

Unverified claims

  • The quoted verification transcript is not a transcript of the final test set — strided-basic/src/raw_ops/tests/tiled_tests.rs:450 and :556 add two tests, but the body still claims “12 passed” for raw_ops::tiled_tests “at this head.” The available parallel-enabled test executable enumerates and passes 14 tests under that filter. Reproduce with cargo test -p strided-basic --lib -- raw_ops::tiled_tests at 587c479c. The quoted counts cannot establish that the final tail-buffer and clipping additions were included in the reported local gates; regenerate the transcript or identify its actual SHA.

  • The rank-12 slowdown is not explained by the quoted absolute tile counts — strided-basic/src/raw_ops.rs:709–718 processes each tile, and the body attributes the residual to “64x as many tiles,” but [4;12] also contains 64 times as many elements as [4;9]. Reproduce the arithmetic: 128 / 4^9 = 8192 / 4^12 = 1 / 2048 tiles per element. This does not disprove rank-dependent walker overhead, but the tile counts do not establish that cause for an ns/element difference. Instrument walker work per element or qualify the causal conclusion.

  • The repository-rules gate is not independent semantic-review evidence — .github/workflows/ci.yml:228–242 runs maintenance-script tests, while AGENTS.md documents that the rules gate checks secret-shaped additions and the deprecated-tree freeze, with its LLM pass disabled. Reproduce with gh pr checks 429 --repo tensor4all/strided-rs: the rules check succeeds while “repository rules review (LLM skipped)” is skipped; the PR overview returned no submitted reviews. The body’s separate read-only review reports cannot be authenticated from those checks or the diff. This does not establish that those reviews did not occur.

Checked and fine

Executed the existing parallel-enabled test binary: all 14 raw_ops::tiled_tests passed; its fingerprint lists default, parallel, and simd.
Did not rebuild or rerun the full workspace, allocation tests, mutation tests, or benchmarks; the existing binary’s correspondence to every current source file was not independently rebuilt.


Reviewed head: 587c479c0a4cb763f12da9849fe8cb7f9fc57d09 at 2026-11-09T09:41:10Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka

shinaoka commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

GPT-6.1-sol adversarial review

CRITICAL: none
NEEDS_CHANGES: 1
UNVERIFIED: 2

Critical (must not merge)

none

Needs changes

  • The allocation guarantee still includes layouts that allocate and then decline tiling — strided-basic/src/raw_ops.rs:478–486, with the decline at :421. Reproduce with dims = [2; 9], both stride arrays [1024; 9], separate f64 buffers of length 9217, offset zero, and replay_copy_raw(..., Exec::serial()). Each operand’s modeled region is 64 × 2^9 = 32768 bytes, so the combined region exceeds the budget; equal strides also fail the strictly-increasing exit. The three vectors are therefore allocated before entering the model, but identical operand stride orders make the model retain every dimension, so first_cut returns None. An allocator-counting reproducer linked against the existing local library observed 9 allocations for a warmed call. This contradicts the body’s “a spilled layout the model keeps whole allocates exactly what it allocated before” and “only a layout the model actually tiles adds buffers” statements. Narrow those statements to the enumerated pre-metadata exits; do not describe every untiled result as allocation-preserving.

Unverified claims

  • Identical block shapes do not establish that the residual cost is exclusively inside the walker — strided-basic/src/raw_ops.rs:478–486,688–718, PR “Measurement” section. The pasted harness times replay_copy_raw, including tile selection and its rank-dependent metadata work, so comparing block vectors does not exclude model/selection cost. Moreover, the explanatory rank-9 vector concerns [4; 9], whereas the timed rank-9 input is [4,4,8,8,8,8,8,8,4]. Reproduce by recording the selected blocks for those actual timed dimensions and timing selection separately from walking. The supplied numbers do not establish the assertion that “whatever costs more is inside the walker … rather than in the model.”

  • The verification summaries cannot all describe the stated head under the shown commands — strided-basic/src/raw_ops/tests/tiled_tests.rs:450,556, PR “Verification” section. The filtered result says 14 passed + 235 filtered out = 249 tests, but the immediately following unfiltered cargo test -p strided-basic --lib claims 247 passed with no feature change shown. Reproduce both commands at d521b0fb84374aa7830891e5c28e392b3a557e8a and regenerate the transcript or identify each result’s actual SHA. The existing local test executable also enumerates 249 tests, but that is not a fresh build establishing head provenance. I cannot read original local gate logs tied to this SHA from the supplied artifacts. At CI inspection, run 42596366879 targeted the exact head but still had unfinished jobs, including the macOS parallel lane; completed jobs did not yet establish an entirely green run.

Checked and fine

none

Not checked: a fresh head build, full workspace gates, mutations, or benchmarks. Local executions used prebuilt artifacts; kache doctor --verify timed out before completing.


Reviewed head: d521b0fb84374aa7830891e5c28e392b3a557e8a at 2026-11-09T09:49:46Z. This review is valid only for that commit: if the head has moved, it is a review of a tree that is not being merged.
Automated (gpt_review.sh). This is an enumeration of counterexamples, not a verdict; the merge decision stays with a human.

@shinaoka
shinaoka merged commit edcf71f into main Nov 9, 2026
20 checks passed
@shinaoka
shinaoka deleted the fix/425-spilled-tiling branch November 9, 2026 09:59
shinaoka added a commit that referenced this pull request Nov 9, 2026
The branch was rebased onto main so that #429's spilled-traversal tiling and the
plan's own rate meet in one replay: the tiles now come from
PairTraversal::tiles, and the rate the untiled replay prices itself at is
threaded through the inline, spilled-inline and spilled-raw arms.
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.

Plan and replay copies above fused rank 8 are not cache-blocked (the #370 defect remains for spilled traversals)

1 participant