[Perf] Tiles 1: _load, _store, _eye_

[hughperkins]

Add the foundation for register-resident 16x16 tile operations:

• _make_tile16x16(dtype) factory creating @qd.dataclass with r0–r15
• _load/_store/_load3d/_store3d with row and column bounds checking
• eye_() for in-place identity initialization
• zeros/eye factories

Issue: #

Brief Summary

Description from Opus:

for the purposes of pr description, describe what is in pr 1, and the good and bad points about pr 1

What's in PR 1

Introduces Tile16x16, a register-resident 16x16 matrix tile distributed across 16 subgroup threads. Each thread owns one row stored in 16 scalar registers (r0-r15). This is the
foundation for blocked linear algebra kernels (Cholesky, triangular solve) that follow in later PRs.
Files changed (614 lines, 4 files):
• _tile16.py (324 lines) -- The tile dataclass with _load/_store (2D), _load3d/_store3d (3D), and eye methods. A _make_tile16x16(dtype) factory creates dtype-specialized
tile types, cached to avoid re-creation. Both f32 and f64 are supported.
• type_check.cpp (4 lines) -- Fixes InternalFuncStmt type checking to preserve polymorphic return types (e.g. subgroupShuffle returning f32 instead of being hardcoded to
i32). Required for tile shuffles to work correctly.
• test_tile16.py (276 lines) -- Tests for zeros, eye, eye-inplace, parametrized load/store (various offsets, window sizes, partial rows/cols), 3D load/store, and array-shape
clamping (both row and column).
• test_utils.py (12 lines) -- Shared skip_if_f64_unsupported helper.

────────────────────────────────────────

Good points

• Correct bounds clamping in both dimensions -- _load/_store clamp both row_end and col_end to the actual array shape, preventing out-of-bounds GPU memory access when the tile
window extends past the array.
• Thoroughly parametrized tests -- Load/store tests cover 8 combinations of offsets, partial rows, partial columns on a 92x92 grid, all crossed with f32/f64. Edge cases (array
narrower/shorter than 16) are explicitly tested.
• Clean internal API -- Everything is private (_tile16.py, _load, _store, eye). The public API (Tile16x16 proxy with slice syntax) is deferred to later PRs, keeping this PR
focused.
• No shared memory needed -- The tile design uses only scalar registers and subgroup shuffles, making it portable across CUDA, Vulkan, and Metal.
• The type_check.cpp fix is minimal and correct -- It removes a hardcoded i32 return type that would silently corrupt f32/f64 shuffle results. The fix relies on ret_type
already being set correctly during IR flatten, which is guaranteed by InternalCallOperation::flatten.

Bad points

• Extremely repetitive -- _load, _store, _load3d, _store3d, and eye each repeat the same pattern 16 times for r0 through r15. The four load/store methods alone are ~220
lines of near-identical code. This is an inherent consequence of the DSL not supporting computed field access, but it makes the file hard to review and maintain.
• _load3d/_store3d are near-duplicates of _load/_store -- The only difference is an extra batch index and arr.shape[2] vs arr.shape[1]. If the DSL supported 2D views
from 3D arrays, these could be eliminated entirely.
• Parameter order is non-standard -- _load(arr, row_start, col_start, col_end, row_end) interleaves row and column parameters instead of grouping them (row_start, row_end,
col_start, col_end). Easy to accidentally swap col_end and row_end when calling the internal API directly.
• _tile16_cache is never invalidated -- The module-level cache persists across qd.reset() / qd.init() cycles. This appears to work in practice because the cached StructType
is just a type descriptor, but it's fragile and undocumented.
• No assertion if row_end - row_start > 16 -- Passing a range larger than 16 rows silently ignores rows beyond row_start + 15. This is correct behavior for a 16-row tile,
but a caller mistake here would be hard to debug.

Fix the parameter order

All call sites now use the new order (arr, row_start, row_end, col_start, col_end).

what are your thoughts on 'No assertion if row_end - row_start > 16 -- Passing a range larger than 16 rows silently ignores rows beyond row_start + 15. This is correct behavior for a 16-row tile, but a caller mistake here would be hard to debug.'?

Thinking about it more, I don't think it's worth adding. A few reasons:

1. It's symmetric with columns. If you pass col_end - col_start > 16, only 16 columns are loaded too (there are only 16 registers). No assertion there either. Adding one for
   rows but not columns would be inconsistent, and adding both would be annoying.
2. The silent clamping is actually convenient. In a tiled loop over a matrix, the last tile often has row_end = total_rows without bothering to compute min(row_start + 16,
   total_rows). The tile naturally handles this by only loading the 16 rows it can hold. Asserting would force every caller to do that clamping manually.
3. The slice syntax in PR 3+ makes it moot. Users write t[:] = arr[r:r+16, c:c+16] and the proxy constructs correct bounds. Nobody calls _load directly except internal code.

I'd remove it from the "bad points" list. It's working as designed.

To Opus: "tell the reviewer a story about PR 1. Assume they know nothing about this code, but they do know high level quadrants concepts, as a user. Walk through: │

• what challenge the PR is aiming to solve
• how it solves it
• how each bit of change code implements this, presneting in a logical order, so that all facts known so far are sufficient to underestand │
  The whole story should be readable in order, without needing to skip ahead to find information that hasn't been presented yet.”

 python/quadrants/lang/simt/_tile16.py lines 44-48

          r0: dtype
          r1: dtype
          // ... r2 through r13 ...
          r14: dtype
          r15: dtype

  The dtype parameter (e.g. qd.f32, qd.f64) comes from a factory function _make_tile16x16(dtype) that creates a specialized tile type for each scalar type. Results are cached to
  avoid recreating the same type:

   python/quadrants/lang/simt/_tile16.py lines 24-29

  def _make_tile16x16(dtype=qd.f32):
      if dtype in _tile16_cache:
          return _tile16_cache[dtype]
      cls = _make_tile16x16_class(dtype)
      _tile16_cache[dtype] = cls
      return cls

  Constructing a tile with Tile() gives you a zero tile (the dataclass defaults all fields to 0). An eye factory creates an identity tile via a module-level function attached to
  the type:

   python/quadrants/lang/simt/_tile16.py lines 317-324

      @qd.func
      def _eye():
          t = result()
          t._eye_()
          return t
      result.eye = _eye
      return result



  Loading and storing

  To be useful, tiles need to move data to/from arrays. Each of the 16 threads loads or stores its own row, so a full tile load/store is 16 parallel row accesses with no
  synchronization needed.
  _load(arr, row_start, row_end, col_start, col_end) reads a rectangular region from a 2D array into the tile's registers. Thread tid loads arr[row_start + tid,
  col_start:col_end]. Both dimensions are cl

              arr_row_end = arr.shape[0]
              if arr_row_end < row_end:
                  row_end = arr_row_end
              row = row_start + qd.simt.subgroup.invocation_id()
              if row < row_end:
                  arr_col_end = arr.shape[1]
                  if arr_col_end < col_end:
                      col_end = arr_col_end

  Each column is then loaded individually with a bounds check (if col_start + i < col_end), since the DSL doesn't support computed field access -- you can't write self.r{i} in a
  loop, so all 16 registers are written out explicitly. _store is the mirror image.
  The 3D variants (_load3d, _store3d) add a batch index for batched operations: arr[batch, row, col] instead of arr[row, col].
  The parameter order (row_start, row_end, col_start, col_end) groups by dimension, matching numpy's arr[row_start:row_end, col_start:col_end] convention.


  The type_check.cpp fix

  To make tiles work with f32 data, subgroup shuffles (used internally by later PRs) need to return the correct scalar type. Before this PR, InternalFuncStmt::visit in
  type_check.cpp hardcoded the return type to i32:

  -    stmt->ret_type = PrimitiveType::i32;
  +    // ret_type is already set correctly during flatten via
  +    // InternalCallOperation::flatten and the operation's type_check.

  The flatten pass already sets the return type correctly from the operation's polymorphic signature (e.g. subgroupShuffle returns ValueT, resolved from the argument type). The
  old code overwrote this with i32, silently corrupting f32/f64 shuffle results. The fix simply stops overwriting it.


  Privacy

  Everything in this PR is internal. The module is named _tile16.py (leading underscore), all methods are prefixed with _, and it's not exported from __init__.py. The user-facing
   API (slice syntax like t[:] = arr[r:r+16, c:c+16] and the Tile16x16 proxy class) comes in later PRs in this chain. This PR establishes the register layout and I/O primitives
  that those later PRs build on.


  Tests

  Tests use the internal _make_tile16x16 factory directly. All tests are parametrized over f32 and f64, with f64 skipped on Metal (unsupported) and Vulkan (unreliable) via a
  shared skip_if_f64_unsupported helper added to test_utils.py.
  The test suite covers:
  • Zero and identity tiles -- verify Tile() produces zeros, Tile.eye() produces identity, and _eye_() overwrites existing data.
  • Load/store round-trip -- parametrized across 8 combinations of source offsets, destination offsets, partial row counts, and partial column counts on a 92x92 grid. Verifies
    that exactly the requested rectangle is copied and nothing else is touched.
  • Array-shape clamping -- loads from / stores to arrays shorter than 16 rows, verifying that excess tile rows stay zero and no out-of-bounds writes occur. (Matching
    column-clamping tests are in the next PR that adds slice syntax.)
  • 3D batch dimension -- parametrized across batch indices, offsets, and partial windows on a (6, 92, 92) array.

copilot:summary

Walkthrough

copilot:walkthrough

[hughperkins]

move this inside each test metdho

[hughperkins]

should this SIZE be private? Or is it the same one we use later? If not it should be priate.

[hughperkins]

STate that everythign is private, because hti is an internal API, wrapped by th elater Tile proxy classes.

[hughperkins]

shouldnt eye_ be private? ie _eye_?

[hughperkins]

why this change?

[hughperkins]

opus:

The old code hardcoded every InternalFuncStmt return type to i32, overriding whatever was set during IR flattening. This breaks subgroupShuffle — the shuffle operation returns
the same type as its input (e.g. f32 for float tiles), but the type checker was forcing it to i32.
Without this fix, every warp shuffle in the tile operations would be type-checked as returning an integer, causing miscompilation. The ret_type is already set correctly by
InternalCallOperation::flatten and the operation's own type_check, so the override just needs to be removed.

[hughperkins]

in this pr are we tesing both f32 and f64?

[hughperkins]

opus:

No — PR 1 only tests f32. The f64 parametrization is in the full test suite introduced in PR 4 (which has @pytest.mark.parametrize("qd_dtype", [qd.f32, qd.f64])).
Do you want me to add f64 coverage to PR 1's tests?

[hughperkins]

oes this pr support both f32 and f64?

[hughperkins]

Opus:

Yes — the code supports both. _make_tile16x16(dtype) accepts any scalar dtype (qd.f32 or qd.f64), and all register fields are declared with the passed dtype. The tests just
only exercise f32. Want me to parametrize the PR 1 tests over both?

[hughperkins]

yes

[hughperkins]

should this be private?

[hughperkins]

Opus:

On PR 1, qd.simt.tile16 is exposed in init.py via lazy loading, but the only consumer is the test file — and the test imports directly from quadrants.lang.simt.tile16, not
through qd.simt.tile16.
So no, it doesn't need to be public in PR 1. We could remove it from all and the getattr lazy loader, deferring that to PR 4 when the public Tile16x16 proxy is
introduced. Want me to make that change?

[hughperkins]

why does Tile16x16 proxy need tile16 to be public?

[hughperkins]

Opus:

It doesn't. Nothing uses qd.simt.tile16 as a public module — every consumer imports directly from the full path quadrants.lang.simt.tile16. The getattr lazy loader for
"tile16" in init.py is dead code.
So we can remove tile16 from all and the getattr block entirely, on all branches. Want me to do that?

[hughperkins]

yes. so then it would not be publicly visible? or do we need to rename to _tile16 too?

[hughperkins]

Opus:

Removing it from all and getattr means qd.simt.tile16 won't resolve — users can't discover or access it through the public API. That's the main thing.
However, from quadrants.lang.simt.tile16 import ... (the full path) would still work because the file tile16.py exists on disk. Python doesn't enforce privacy for module
files. Renaming to _tile16.py would make the "don't use this" convention stronger, but it's just a convention — both are still importable.
Given that everything inside the module is already prefixed with _ (_make_tile16x16, _TILE, _Tile16x16, etc.), I'd say removing from all/getattr is sufficient.
Renaming the file would require updating imports across ~5 files on every branch — a lot of churn for a convention signal.
Want to just remove it from the public surface, or also rename the file?

[hughperkins]

rename the file

[hughperkins]

why is this on type: ignore?

[hughperkins]

Opus:

Every file in quadrants/lang/simt/ has it. It's a convention for this package — these modules define GPU kernel functions using @qd.func/@qd.dataclass with type annotations
like dtype (a runtime variable) as field types, which don't make sense to mypy/pyright. The # type: ignore suppresses type checker errors across the whole file.
It was there in the original branch; not something we added.

[hughperkins]

no, it's not a convention. remove it please, and make sure pyright passes

[hughperkins]

why i0? why not just i?

[hughperkins]

Opus:

No good reason — it was named to match the old row0/col0 convention. Now that those are row_start/col_start, i0 is inconsistent. Should I rename it to batch?

[hughperkins]

yes, good

[hughperkins]

why do we need qd.i32?

[hughperkins]

why is it ok to remove this comment?

[hughperkins]

Some queitons to opus:

question: is there any code that is duplicate by or in this PR that we could refactorize better somehow?

Looking at _tile16.py on hp/tiles-1, the major duplication is:

1. _load, _load3d, _store, _store3d — four methods with near-identical structure. Each has 16 guarded field accesses. They differ only in load-vs-store direction and
   2D-vs-3D indexing. That's ~4 x 48 = ~192 lines of boilerplate.
2. _eye_ — 16 lines zeroing + 16 conditional lines setting diagonal = 32 repetitive lines.

The question is what can actually be refactored given the Quadrants DSL constraints:
• The 16 per-register lines within a method: Can't be looped because self.r0..self.r15 are individual named fields, not an array. The Quadrants compiler processes the AST and
needs to see each field access explicitly.
• _load3d in terms of _load: If Quadrants supported 2D views of 3D arrays (arr[batch, :, :]), then _load3d would just call _load. Does it?
• _load vs _store: These are inverses of each other (self.r0 = arr[...] vs arr[...] = self.r0). Hard to unify without higher-order functions in the DSL.
• Code generation: We could generate all four methods programmatically at the Python level (before qd.dataclass sees them), collapsing ~200 lines of source into ~20 lines of
generation logic. The generated code would be identical, but the source wouldn't be repetitive.

Want me to try the code generation approach? Or check if 3D-to-2D slicing works first?

Is there any code or code change in this PR that might be embarrassing in some way if published in a public github repo?

A few things that would catch a reviewer's eye:

1. col_start + 0 — appears 4 times. It's col_start. The + 0 is there for visual alignment with + 1, + 2, etc., but it looks like the author doesn't know that adding zero is a
   no-op.
2. qd.i32(qd.simt.subgroup.invocation_id()) — as we just discussed, invocation_id() already returns i32. The redundant cast suggests the author didn't check the return type.
3. Test variables named col_stop/row_stop while the API parameters are col_end/row_end — inconsistent naming between the API and its tests.
4. dst_row_dx/dst_col_dx — unusual naming. row_offset/col_offset would be clearer.

No secrets, offensive content, or anything truly bad. Want me to clean these up?

yes please

⬢ To-do All done
☒ Remove col_start + 0 → col_start in _tile16.py
☒ Remove redundant qd.i32() casts in _tile16.py
☒ Rename col_stop/row_stop → col_end/row_end in tests
☒ Rename dst_row_dx/dst_col_dx → row_offset/col_offset in tests
☒ Run tests, commit, push, propagate

Done. All cleaned up and propagated:
• Removed col_start + 0 → just col_start
• Removed unnecessary qd.i32() casts
• Renamed dst_row_dx/dst_col_dx → row_offset/col_offset
• Renamed col_stop/row_stop → col_end/row_end to match the API

Are there any layout inconsistencies in this code relative to itself and to other existing repo code?

Here's what I found:

1. Section separator banner (# ====...====) — only one other file in the entire python/quadrants/ tree uses these (primitive_types.py), and it uses shorter ones. The other simt
   files (subgroup.py, block.py, warp.py) don't use any. Feels out of place.
2. Unicode characters — em dashes (—) and en dashes (–) in the docstrings/comments (lines 10, 22, 31). The rest of the codebase almost certainly uses plain ASCII (-- or -).
3. Duplicate _skip_if_f64_unsupported — defined in both test_tile16.py and test_simt.py with slightly different behavior (ours skips Vulkan unconditionally, test_simt.py only
   skips Vulkan on macOS). Duplication and inconsistent skip logic.
4. No blank line between # pyright: directive and the module docstring on line 2. Minor but looks cramped.

Want me to fix these — remove the banner, replace Unicode with ASCII, add the blank line, and factor out the f64 skip helper?

yes

⬢ To-do All done
☒ Remove section separator banner in _tile16.py
☒ Replace Unicode dashes with ASCII in _tile16.py
☒ Add blank line after pyright directive
☒ Factor out _skip_if_f64_unsupported to test_utils or shared location
☒ Run tests, commit, push, propagate

Done. All branches pushed with:
• Section banner removed
• Unicode dashes replaced with --
• Blank line added after pyright directive
• skip_if_f64_unsupported moved to test_utils and reused from tests

Are there any coding inconsistencies in this code relative to itself and to other existing repo code?

Here's what I found. The main coding inconsistency is:
Decorator ordering: In test_tile16.py, @test_utils.test is placed before @pytest.mark.parametrize:

@test_utils.test(arch=qd.gpu)
@pytest.mark.parametrize("qd_dtype", _QD_DTYPES)
def test_tile16_zeros(qd_dtype):

But 94.5% of tests in the repo (189 out of 200 instances across 61+ files) use the opposite order -- @pytest.mark.parametrize first, then @test_utils.test:

@pytest.mark.parametrize("qd_dtype", _QD_DTYPES)
@test_utils.test(arch=qd.gpu)
def test_tile16_zeros(qd_dtype):

This affects every parametrized test in test_tile16.py on hp/tiles-1 (and carries through to all downstream branches).
This same inconsistency is also present in hp/tiles-4's expanded test file, along with a few more issues that we already fixed in tiles-1 but haven't propagated yet
(_skip_if_f64_unsupported still local, dst_row_dx naming, section banners, M without underscore prefix).
Want me to fix the decorator ordering on hp/tiles-1 and propagate?

yes

⬢ To-do All done
☒ Fix decorator ordering in hp/tiles-1 test_tile16.py
☒ Commit and push hp/tiles-1
☒ Propagate fix through hp/tiles-2a, 2b, 3, 4, 5

Done. All branches updated and pushed:
• hp/tiles-1: Swapped @test_utils.test / @pytest.mark.parametrize ordering to match the repo convention (parametrize first, then test_utils.test).
• hp/tiles-2a, 2b, 3: Clean rebases, no conflicts.
• hp/tiles-4: Resolved conflict in the full test file replacement, and also fixed the same set of inconsistencies that were already fixed in tiles-1 (local
_skip_if_f64_unsupported -> test_utils.skip_if_f64_unsupported, dst_row_dx -> row_offset, M -> _M, removed section banners, removed unnecessary qd.i32() casts, replaced
Unicode dashes with ASCII).
• hp/tiles-5: Clean rebase on top of tiles-4.

All branches pass pre-commit.

Is there anything in this PR that could be implemented in less code?

The obvious candidate is the massive repetition: _load, _load3d, _store, _store3d, and eye each repeat the same pattern 16 times for r0 through r15. Together that's ~230
lines of the 312-line file.
There are two angles:

1. Can _load3d/_store3d delegate to _load/_store?
   The 3D variants are identical to the 2D ones except arr[batch, row, col] vs arr[row, col] and arr.shape[2] vs arr.shape[1]. If the DSL supported something like arr[batch]
   returning a 2D view, _load3d would be a one-liner. But compiled GPU DSLs typically don't support that kind of view construction.
2. Can the 16-line register repetition be looped?
   The DSL has qd.static(range(16)) for compile-time unrolling, but even with a static loop variable i, you can't write self.r{i} -- field access is resolved by name at AST parse
   time, not by string interpolation.
   You could code-generate the method bodies in Python at class-definition time (using exec() or AST construction), turning 16 hand-written lines into a 2-line loop. But that
   trades verbosity for readability -- the generated code would be harder to debug and inspect.
   So the bottom line: the repetition is an inherent consequence of having 16 individually-named scalar register fields in a DSL that doesn't support computed field access. It's
   verbose but intentional -- every similar register-tile implementation I've seen (WMMA wrappers, etc.) has this same pattern. There's no easy way to cut it down without
   sacrificing readability or requiring DSL changes.

[hughperkins]

(continuing in new comment, since previous comment getting unwiedly)

Is there anything in this PR that could be implemented in a way that will run faster at runtime?

The runtime-critical code is in _tile16.py. Let me look at the _load/_store methods specifically.
The hot path is the 16 per-element bounds checks in _load and _store:

/home/hugh/ais/seagull/quadrants/python/quadrants/lang/simt/_tile16.py lines 62-74

      @qd.func
      def _load(self, arr: qd.template(), row_start, col_start, col_end, row_end):
          # ...
          row = row_start + qd.simt.subgroup.invocation_id()
          if row < row_end:
              arr_col_end = arr.shape[1]
              if arr_col_end < col_end:
                  col_end = arr_col_end
              if col_start < col_end:
                  self.r0 = arr[row, col_start]
              if col_start + 1 < col_end:
                  self.r1 = arr[row, col_start + 1]
              # ... 14 more branches ...

Every load/store does 16 individual if col_start + i < col_end checks. For the common case (full 16-column tile load), all 16 checks pass and they're pure overhead.
A fast path could skip them entirely:

if col_start + 16 <= col_end:
# Full tile: no per-element checks needed
self.r0 = arr[row, col_start]
self.r1 = arr[row, col_start + 1]
...
self.r15 = arr[row, col_start + 15]
else:
# Partial: check each element
if col_start < col_end:
self.r0 = arr[row, col_start]
...

That said, in practice this is likely a marginal win: col_end is typically uniform across the subgroup, so the GPU handles these as predicated instructions rather than true
divergent branches. The compiler may already be optimizing this well.
Everything else looks fine -- invocation_id() is called once per method, no redundant memory accesses, no unnecessary synchronization. The memory access pattern (each thread
reads its own row) is inherent to the row-per-thread design and can't be changed without redesigning the tile layout.
Short answer: the only candidate is adding a fast path for full-tile loads/stores, but it doubles the code size and the GPU compiler likely already handles the uniform-branch
case efficiently. Probably not worth it unless profiling shows it's a bottleneck.

Is there anything in this PR which might exhibit incorrect behavior in some way?

  Yes, there's an asymmetry in bounds checking that could lead to incorrect behavior:
  Column bounds are clamped to the array shape, but row bounds are not.
  In _load:

   /home/hugh/ais/seagull/quadrants/python/quadrants/lang/simt/_tile16.py lines 68-72

              row = row_start + qd.simt.subgroup.invocation_id()
              if row < row_end:
                  arr_col_end = arr.shape[1]
                  if arr_col_end < col_end:
                      col_end = arr_col_end

  If a caller passes row_end > arr.shape[0], and a thread's row_start + tid is < row_end but >= arr.shape[0], it reads arr[row, ...] out of bounds. On a GPU this is undefined
  behavior -- could return garbage or crash.
  The column dimension is explicitly clamped to arr.shape[1] (line 70-72), but no equivalent clamping exists for rows:

  # Columns: clamped (safe)
  arr_col_end = arr.shape[1]
  if arr_col_end < col_end:
      col_end = arr_col_end
  # Rows: NOT clamped (caller must ensure row_end <= arr.shape[0])

  The same asymmetry exists in _store, _load3d, and _store3d.
  In practice this is mitigated because:
  • PR 3+ introduces slice syntax that constructs bounds from the array shape
  • The tests in PR 1 use correctly-sized arrays

  But the raw _load/_store API has this inconsistency. Either rows should also be clamped (matching the column behavior), or columns should not be clamped (matching the row
  behavior and making the caller fully responsible). The current halfway approach is the most dangerous because it gives a false sense of safety.
  Want me to add row clamping to match the column clamping?

yes, and also unit tests for this

⬢ To-do All done
☒ Add row clamping to _load, _store, _load3d, _store3d in _tile16.py
☒ Add unit tests for row clamping (load/store beyond array rows)
☒ Commit, push, propagate through all branches

Done. Here's a summary of the changes:
_tile16.py -- Added row clamping to all four methods, symmetric with the existing column clamping:
• _load: arr_row_end = arr.shape[0]; if arr_row_end < row_end: row_end = arr_row_end
• _store: same pattern with arr.shape[0]
• _load3d: same pattern with arr.shape[1] (row dim in 3D)
• _store3d: same pattern with arr.shape[1]

test_tile16.py (hp/tiles-1) -- Added 4 new tests:
• test_tile16_load_clamp_to_array_rows -- loads from a 10-row array with row_end=16, verifies rows 10-15 are zero
• test_tile16_store_clamp_to_array_rows -- stores to a 10-row array with row_end=16, verifies no OOB write
• test_tile16_load3d_clamp_to_array_rows -- same for 3D
• test_tile16_store3d_clamp_to_array_rows -- same for 3D

test_tile16.py (hp/tiles-4) -- Added 2 proxy-API equivalents:
• test_tile16_load_clamp_to_array_rows
• test_tile16_store_clamp_to_array_rows

All branches rebased, pre-commit passes, force-pushed.

[hughperkins]

check the test coverage of the tests. Is there any code that is being added or modified in this PR, that is not being exercised by the tests? Is there any functionality that is implied by consistency or otherwise that is not being tested by these tests?

Here's the coverage analysis:
Code added but not exercised by any test:

1. result.zeros = result (line 315) -- The .zeros factory alias is never used. Every test constructs tiles with Tile(). No test calls Tile.zeros().
2. result.SIZE = _TILE (line 313) -- No test checks that Tile.SIZE == 16. It's set but never read.
3. result._quadrants_internal = True (line 314) -- This flag tells the AST purity checker to exempt tile code from global-access restrictions. Not testable in PR 1 since the
   purity exemption logic comes in PR 3.
4. type_check.cpp change -- The fix to InternalFuncStmt preserves polymorphic return types for operations like subgroupShuffle. But PR 1's tests never trigger a shuffle. The
   fix is needed for later PRs (Cholesky uses shuffles on f32 values), so it's dead code from PR 1's perspective.

Functionality implied by consistency but not tested:
5. Column clamping to array shape -- Row clamping is tested (10-row array, row_end=16, verifying extra rows are zero). But there's no equivalent test for a narrow array --
loading from a (16, 10) array with col_end=16 and verifying that columns 10-15 stay zero. The column clamping code exists (lines 73-75) but only gets exercised for the
narrow-array case in PR 4.
6. Store partial columns, verify untouched columns -- test_tile16_load_store with ncols=5 loads 5 columns, then stores 5 columns. But the destination is pre-filled with -1.0
and the assertion checks the whole grid, so it implicitly verifies untouched columns stay -1.0. However, there's no test that loads a full 16 columns and then stores only a
subset (which would verify that _store's column bounds independently clamp, separate from _load's).
7. _eye_() on a zero tile -- test_tile16_eye_inplace loads non-zero data then calls eye(), verifying the overwrite. But Tile.eye() calls eye() on a freshly zeroed tile.
There's no test that verifies eye() works correctly on a zero tile specifically (though test_tile16_eye implicitly tests this via Tile.eye()).
8. _make_tile16x16 caching -- The cache is exercised (multiple tests call _make_tile16x16(qd.f32), hitting the cache on subsequent calls), but no test verifies that the
cached object is the same instance, i.e., _make_tile16x16(qd.f32) is _make_tile16x16(qd.f32).

Most impactful gaps to close would be items 1 and 5:
• A one-line test for Tile.zeros() (since it's an alias exposed by the API)
• A column-clamping test mirroring the existing row-clamping test

Want me to add these?

Request to Opus: " │ → - remove result._quadrants_internal = True │
│ - add test for Tile.SIZE == 16 │
│ - add test for Tile.zeros() │
│ - move ``type_check.cpp change to a later PR (or maybe a new PR, lke '1b' perhaps) │
│ - add test for 'Column clamping to array shape" │
│ - add check for 'Store partial columns, verify untouched columns' │
│ - add test for '`_make_tile16x16` caching' │
│ │
│ Make a todo list ofr htese, then go through one by one"

All done. Here's what was completed:

1. Removed result._quadrants_internal = True from _tile16.py on hp/tiles-1 -- it's not consumed until PR 3 where ast_transformer.py checks it. Re-added it in hp/tiles-3 with
   a dedicated commit.
2. Added test_tile16_size_constant -- asserts Tile.SIZE == 16.
3. Added test_tile16_zeros_factory -- creates a tile via Tile.zeros(), stores it, and verifies all zeros (parametrized f32/f64).
4. Moved type_check.cpp change to hp/tiles-1b -- new branch forked from hp/tiles-1 containing only the InternalFuncStmt fix. The chain is now: main -> hp/tiles-1 ->
   hp/tiles-1b -> hp/tiles-2a -> ...
5. Added test_tile16_load_clamp_to_array_cols -- loads from a (16, 10) array into a 16x16 tile, verifies columns beyond width are zero (parametrized f32/f64).
6. Added test_tile16_store_partial_cols_untouched -- loads full 16 columns, stores only 10, verifies remaining destination columns are untouched at -1.0 (parametrized
   f32/f64).
7. Added test_tile16_make_caching -- verifies _make_tile16x16(f32) is _make_tile16x16(f32) and f32 is not f64.

[hughperkins]

check the factorization of the tests. are there some tests that could be merged, e.g. making use of parametrize?

Here's my analysis of merge opportunities:

1. test_tile16_zeros + test_tile16_zeros_factory (lines 13-30, 284-302)
   These are nearly identical — same kernel shape, same assertion (all zeros). The only difference is Tile() vs Tile.zeros(). However, since these are compiled GPU kernels, you
   can't easily parametrize the constructor call inside the kernel body. They'd need two separate kernels regardless. Verdict: marginal gain, reduced clarity.
2. test_tile16_load_clamp_to_array_rows + test_tile16_store_clamp_to_array_rows (lines 127-177)
   These use the exact same kernel (_load then _store with full bounds). They differ only in which array is undersized (src vs dst) and in the assertions. Could be merged with a
   parametrize on (src_shape, dst_shape) and conditional assertions. Same for the 3D pair (lines 180-230). Verdict: best candidate — same kernel, 4 tests could become 2.
3. test_tile16_load_clamp_to_array_cols + test_tile16_store_partial_cols_untouched (lines 305-357)
   These use different kernels (different col_end in _store), so they can't share a kernel. Verdict: not mergeable.
4. test_tile16_eye + test_tile16_eye_inplace (lines 33-73)
   Different things: factory vs in-place mutation of a loaded tile. Verdict: not mergeable, testing distinct behavior.
   The only real win is merging the row-clamping pairs: collapse 4 tests into 2 by parametrizing on which side (src or dst) is short. That said, it would slightly reduce
   readability since the assertions differ per case. Want me to do it, or do you think the current factorization is acceptable?

reminder: you can use if qd.static(...) in quadrants kernels to switch between different compiled code.

Good point — that changes the picture. With qd.static(), more tests can share a single kernel via compile-time branching. Updated analysis:
Mergeable with qd.static:

1. test_tile16_zeros + test_tile16_zeros_factory — parametrize use_zeros_alias, branch Tile() vs Tile.zeros() via qd.static
2. test_tile16_eye + test_tile16_eye_inplace — parametrize inplace, branch Tile.eye() vs Tile() + _load + eye() via qd.static

Mergeable without qd.static (same kernel):
3. load_clamp_to_array_rows + store_clamp_to_array_rows — same kernel, parametrize on which side is short
4. load3d_clamp_to_array_rows + store3d_clamp_to_array_rows — same kernel, parametrize on which side is short

That would reduce 8 test functions → 4. Let me do it.

Done. Merged 8 test functions into 4 by:

1. test_tile16_zeros + test_tile16_zeros_factory → single test_tile16_zeros with use_zeros_alias param, using qd.static to branch between Tile() and Tile.zeros()
2. test_tile16_eye + test_tile16_eye_inplace → single test_tile16_eye with inplace param, using qd.static to branch between Tile.eye() and Tile() + _load + eye()
3. test_tile16_load_clamp_to_array_rows + test_tile16_store_clamp_to_array_rows → single test_tile16_clamp_to_array_rows with clamp_side param (same kernel, different array
   shapes)
4. test_tile16_load3d_clamp_to_array_rows + test_tile16_store3d_clamp_to_array_rows → single test_tile16_3d_clamp_to_array_rows with clamp_side param

Net: -72 lines. The column-clamping tests (load_clamp_to_array_cols and store_partial_cols_untouched) were left separate since they use different kernels (different col_end in
_store) and test conceptually different clamping mechanisms.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2988a5b2e9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Derive tile row from block-local thread id

Using subgroup.invocation_id() to choose the tile row assumes a subgroup width of at least 16, but this is not guaranteed (the repo’s SIMT tests only assume a minimum subgroup size of 4). On GPUs where a 16-thread block is split into smaller subgroups (e.g., 2×8 lanes), lane IDs repeat per subgroup, so _load/_store/_load3d/_store3d/_eye_ operate on rows 0–7 twice and never touch rows 8–15, corrupting tile I/O and identity initialization.

Useful? React with 👍 / 👎.

[hughperkins]

This is a great observation by Claude.

This is being addressed in #455

[claude]

🟡 The new shared skip_if_f64_unsupported() helper in test_utils.py skips Vulkan f64 on all platforms, but the pre-existing _skip_if_f64_unsupported() in test_simt.py only skips on macOS (where MoltenVK is the Vulkan implementation). This causes tile16 f64 tests to be silently skipped on Linux Vulkan even though Vulkan natively supports f64 there — a test coverage regression. Fix by adding and platform.system() == 'Darwin' to the Vulkan branch, matching test_simt.py's established convention.

Extended reasoning...

What the bug is

tests/test_utils.py lines 334-335 (the new shared helper) unconditionally skip all Vulkan f64 tests:

if arch == qd.vulkan:
    pytest.skip("Vulkan does not reliably support f64")

But the pre-existing _skip_if_f64_unsupported() in test_simt.py lines 19-20 only skips on Darwin:

if arch == qd.vulkan and platform.system() == 'Darwin':
    pytest.skip("MoltenVK does not support f64")

The rationale in test_simt.py is precise: the problem is MoltenVK (the macOS Vulkan implementation), which does not support f64. Native Vulkan on Linux has no such limitation.

The specific code path

Every tile16 f64 test in test_tile16.py calls test_utils.skip_if_f64_unsupported(qd_dtype) at the top. When the test runner is configured to use Vulkan on Linux (which conftest.py does), this call reaches the new helper, hits the unconditional arch == qd.vulkan branch, and issues pytest.skip — regardless of whether the platform is Linux, where f64 Vulkan actually works.

Why existing code doesn't prevent it

The PR author was aware of the discrepancy (PR timeline: "ours skips Vulkan unconditionally, test_simt.py only skips Vulkan on macOS") but chose to be more conservative. No justification is given for why tile16 operations would fail on Linux Vulkan when test_simt.py shuffle f64 tests (which exercise the same underlying subgroup intrinsics) continue to pass there.

Impact

All tile16 f64 tests (test_tile16_zeros, test_tile16_eye, test_tile16_load_store, test_tile16_clamp_to_array_rows, etc.) are silently skipped on Linux Vulkan. Meanwhile, test_simt.py f64 tests run normally on the same platform with the same backend. This is a silent test coverage regression — CI stays green while f64 tile16 coverage is absent on Linux Vulkan.

Step-by-step proof

1. Linux CI runner with Vulkan enabled runs pytest.
2. test_tile16_zeros[qd_dtype=qd.f64-arch=vulkan] is invoked.
3. test_utils.skip_if_f64_unsupported(qd.f64) is called.
4. dtype \!= qd.f64 is False, so execution continues.
5. arch = qd.lang.impl.current_cfg().arch returns qd.vulkan.
6. arch == qd.vulkan is True → pytest.skip("Vulkan does not reliably support f64") fires.
7. Test is marked as skipped with no failure, no warning.
8. The same runner executes test_simt.py f64 shuffle tests with Vulkan — those use _skip_if_f64_unsupported which checks platform.system() == 'Darwin' → False on Linux → tests run and pass.

How to fix

Change the Vulkan branch in test_utils.skip_if_f64_unsupported to match the convention established in test_simt.py:

if arch == qd.vulkan and platform.system() == 'Darwin':
    pytest.skip("MoltenVK does not support f64")

This restores f64 tile16 coverage on Linux Vulkan while still correctly skipping on macOS where MoltenVK is the implementation.

[hughperkins]

This is intentional. Some tests were failing wiht f64, even on linux vulkan.

[erizmr]

Approving on the basis that I have reviewed the design and the public facing API, and they look reasonable to me.