Repository navigation
basic: tile a spilled pair traversal (rank >= 9) as its compressed layout would be tiled - #429
Conversation
…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'.
Repository rules reviewRepository 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. |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not run: Cargo tests, targeted mutations or benchmarks; Reviewed head: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Reviewed head: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked locally: Cargo tests, mutation runs, or benchmarks; runtime correctness and performance beyond the inspected evidence remain unverified. Reviewed head: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Reviewed head: |
…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
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Reviewed head: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fineCI run 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: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fineExecuted the existing parallel-enabled test binary: all 14 Reviewed head: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: a fresh head build, full workspace gates, mutations, or benchmarks. Local executions used prebuilt artifacts; Reviewed head: |
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.
Closes #425.
#422 cache-blocked plan and replay copies, but only for inline traversals (fused rank ≤ 8). A
PairTraversal::Spilledtraversal still walked untiled throughapply_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
strided-basic/src/block.rs(exits_before_metadata,region_of,axes_region)raw_ops.rs(tile_blocks)PairTraversal::tiles— the single decision entry point a plan and a replay entry both callraw_ops.rsspilled_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 modelraw_ops.rsSpilledTiles+apply_spilled_tiled_box: the model's blocks beside rank-sized stride slices, andapply_tiled_box's tile odometer over themraw_ops.rsreplay_spilled_tiled: the view path's tree splitter, one box walk per leafcopy_plan.rsreplay_fuseddispatch (Tiles::Inlinefrom a spilled list goes through the inline walk unchanged)copy_plan.rsslabs_share_no_line's fixed 8-entry tail → inline up to 8, heap above it (latent panic above 8)raw_ops.rsCopyPlan/UpdatePlanreplay andreplay_given(hence the fourreplay_{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_givenstill drops the tiles and limits the token to one slot for a destination it cannot prove injective, and both plan compilers reject one. The statelesscopy_scale_raw/axpy_rawfamily (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_metadataanswers 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 sameregion_of/axes_regionarithmetic asmemory_region,u64with the wide fallback;spilled_pair_tilesconsults 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/UpdatePlanexecution: the rank-sized range cursor per execution (32 for 16execute+ 16execute_scalecalls on the[2;9]reversed-stride fixture, the same number as the base commit);replay_*_raw: the axis list plus that cursor (2 per call, same as the base commit);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) = 56bytes → 1 contiguous line, and the 6 axes of extent 2 at ≥ 64-byte strides →2^6line-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 whatalloc_matrix's restored<= 2andcopy_plan_alloc's restored32now assert.Measurement — the rank-9 cliff is gone, and the issue's no-cliff bar is not met
Harness (a
b425binary with a path dependency on the reviewed worktree,--release, nothing else):Raw sample, base commit
b2358fb(before),taskset -c 0, hostAMD Ryzen AI MAX+ 395(EVO-X2), 32 CPUs, 2 × 64 MiB L3 (0-7,16-23/8-15,24-31), load average 0.57:Raw sample, this branch (
taskset -c 0, four consecutive runs, load average 0.78 → 1.66). One run in full:The other three runs as printed, ratio line only, so the spread is visible rather than one cherry-picked sample:
So, against the issue's proposed acceptance:
[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 timesreplay_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
@stridedratio, 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 throughOption<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_tilesreturningNone, everything else unchanged):Red second, for the prepared-plan entries specifically: suppress tiles only for a spilled traversal in the two plan
replaybodies, leaving the plan-free entries alone.Red third, for the early exit: evaluate the region in
u64only, as if an overflow meant "does not fit".All three mutations are reverted. Green, at this head:
Coverage of the new route:
layouts()gained4^9 reversed, a rank-12reversedlayout, 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_uninitrows, f64 and Complex64, serial and team) runs against the independent per-element oracle through the spilled tile walk.spilled_plan_entries_visit_tilespins each prepared-plan entry to an executed tile count,spilled_layouts_tile_as_their_compressed_rank_woulddoes 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_uninitwas missing from the generic entry/type matrix — fixed by adding that row tocheck_entries, so it now runs for f64 and Complex64 on serial and team tokens. Its delete-list item, a redundantrank < 2guard inspilled_pair_tiles(pair_tilesalready declines fewer than two moving axes), is deleted.Round 2 findings, and what closed each one:
block::exits_before_metadataover an axis iterator, withregion_of/axes_regionsharing the model's own region arithmetic) and allocating only after.copy_plan_allocis back at 32 andalloc_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.spilled_plan_entries_visit_tilesnow pins an executed tile count forCopyPlan::execute,execute_scale,execute_conj,UpdatePlan::execute, and both_uninitvariants;spilled_layouts_tile_as_their_compressed_rank_woulddoes 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.u64region 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 inaxes_region, and pinned here bycheck_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
copy_scale_raw/axpy_rawfamily keeps its sequential spilled walk;