Skip to content

fix: chained diagonal_view pairs, large three-axis injectivity, inline grouped-tile tables (#329, #342, #337) - #344

Merged
shinaoka merged 3 commits into
mainfrom
fix/329-342-337
Oct 30, 2026
Merged

shinaoka merged 3 commits into
mainfrom
fix/329-342-337

Conversation

@shinaoka

Copy link
Copy Markdown
Member

Closes #329, closes #342, closes #337.

Three correctness fixes, each reproduced by a failing test before the fix. No public signature, type or error variant changes, so this can ship as a 0.9.x patch.

#329: diagonal_view with chained axis pairs

Reproduced. For dims [3, 3, 3] and strides [1, 3, 9]:

  • [(0, 1), (1, 2)] returned stride [4] and get(&[2]) == 8.0.
  • The reversed order, [(1, 2), (0, 1)], returned [13].

Fix. The pairs now split the axes into classes using union-find. Each class collapses onto its lowest axis, which keeps the summed stride and the minimum dim. The result no longer depends on pair order or on how pairs chain.

Behaviour change, documented in the rustdoc. Two inputs used to be rejected, but only because a double-counted stride happened to overrun the backing:

  • [(0, 2), (1, 2)] now returns the A[i,i,i] diagonal it describes.
  • A repeated pair such as [(0, 1), (0, 1)] is now treated the same as a single pair.

The old test that expected rejection for these is replaced. Each new test asserts the stride and an element value for several pair orders.

#342: is_injective_layout above 2^24

Reproduced. is_injective_layout(&[2, 2, 5_000_000], &[4, 5, 6]) returned false.

Fix. A new allocation-free difference search decides blocks of three or more axes.

  • It enumerates difference vectors only over the axes other than the two longest.
  • For each vector, it solves the two longest axes in closed form, as a bounded two-variable linear Diophantine equation (gcd plus one modular inverse).
  • Its cost therefore does not depend on the long extents: the reproducer visits 4 vectors.
  • The search is budgeted at 2^22 vectors (an INVARIANT comment on the constant).

What remains undecided. A block is still reported false without a decision only when both of these hold:

  • the difference search exceeds 2^22 vectors (this needs three or more long interleaved axes), and
  • the block has more than 2^24 elements.

This bound is now stated on is_injective_layout and NonInjectiveOutputLayout, as the issue allowed.

Tests.

  • A brute-force test checks the search against direct enumeration on every 3- and 4-axis layout with extents 2..4 and strides 1..5, including negative strides (more than 1000 layouts).
  • The allocation-free path now also accepts the old [2, 2, 2000] / [4, 5, 6] case, so that test was updated to expect acceptance.

#337: grouped-tile tables spill at rank 8

Reproduced. After changing perm_alloc to expect == 0, the [100, 2, …] layout made 2 allocations per copy_into.

Fix. Offset tables now cover only the leading group axes, up to GROUP_OFFSETS_INLINE entries.

  • An axis that does not fit is divided at its largest divisor that does. For example, 100 becomes 50 table entries × an outer extent of 2.
  • The rest of the group is walked as outer loops with plain strides.
  • The tables are now ArrayVec, so they never spill.
  • A group that fits the table still runs the existing two-table loop, unchanged.

Tests.

  • perm_alloc asserts zero allocations for both rank-8 layouts that used to spill.
  • A unit test checks the split, including the prime 101-element axis, which is walked whole as an outer loop.
  • tests/issue_337_grouped_tile_split.rs compares the split path against a direct gather in serial and pooled execution.

Not measured. I did not run benchmarks, because broccoli is running a measurement job (faer#160). Layouts whose groups fit the table run the same code as before. Layouts above the bound used to allocate and fill a table of the whole group on every call; they now fill at most 64 entries plus outer loops. Their tile loop is shorter, though. For example, a prime extent above 64 gets an inner run of 1. I'm assuming no regression for these layouts; a benchmark can confirm that.

Verification

All builds ran under nice -n 19 taskset -c 56-63 with -j 8. broccoli has only CPUs 0-63, and the bench job is pinned to 0-15.

  • cargo fmt --all -- --check: OK
  • cargo test --workspace: 1098 passed, 0 failed
  • cargo clippy -p strided-view -p strided-perm --all-targets: no new warnings in the changed code
  • python3 scripts/repository-rules-review.py --base origin/main --worktree --dry-run: pass, no findings

🤖 Generated with Claude Code

shinaoka and others added 3 commits October 30, 2026 11:21
…329)

Chained pairs such as [(0, 1), (1, 2)] dropped the stride of the axis whose
removal was already queued, so the result depended on pair order and returned
an in-bounds but wrong view. Pairs now partition the axes into classes with
union-find; each class collapses onto its lowest axis with the summed stride
and the minimum dim. [(0, 2), (1, 2)] and a repeated pair, previously rejected
only because the double-counted stride overran the backing, now return the
diagonal they describe.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
is_injective_layout still rejected an injective interleaved block of three
or more axes above 2^24 elements, e.g. dims [2, 2, 5_000_000] with strides
[4, 5, 6], so copy_into returned NonInjectiveOutputLayout for a legal
destination. A new allocation-free difference search enumerates difference
vectors over all but the block's two longest axes and solves those two in
closed form (bounded two-variable Diophantine equation), so its cost does not
depend on the long extents. The remaining undecided case (search above 2^22
vectors and block above 2^24 elements) is now documented on
is_injective_layout and NonInjectiveOutputLayout. A brute-force test checks
the search against enumeration on every small three- and four-axis layout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…337)

A grouped tile whose group exceeded GROUP_OFFSETS_INLINE (64) entries spilled
its offset tables to the heap, so a rank-8 copy_into could allocate per call,
against the #306 rank/operand-bounded no-allocation contract. The tables now
cover only the leading group axes up to the inline bound; an axis that does
not fit is divided at its largest divisor that does, and the rest of the
group is walked as plain strided outer loops. A group that fits keeps the
existing two-table loop unchanged. perm_alloc now asserts zero allocations
for the two rank-8 layouts that used to spill, and a new integration test
checks the split path (including a prime 101-element axis) against a direct
gather in serial and pooled execution.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shinaoka shinaoka self-assigned this Oct 30, 2026
@github-actions

Copy link
Copy Markdown

Repository rules review

Repository rules review (6bdf8e757b701ea35d1de09db35112386cbf5876...4feb25dc2645b45df7e67f65d6a064e62b3fafe1)
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 merged commit 50dca66 into main Oct 30, 2026
11 checks passed
@shinaoka
shinaoka deleted the fix/329-342-337 branch October 30, 2026 02:46
shinaoka added a commit that referenced this pull request Oct 30, 2026
The fixes merged in #344 (#329 chained diagonal_view pairs, #342 large
three-axis injectivity, #337 inline grouped-tile tables) touch only
strided-view and strided-perm, with no public API change, so those two get
a patch release. strided-view now carries its own version (0.7.1) instead
of the workspace's, which strided-traits keeps at 0.7.0 because it did not
change. The dependents' requirements (^0.7.0, ^0.9.0) already admit the
patch versions and are left as they are.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment