Blocked-indirection load store, fast path - #188
Conversation
|
(Manually written, based on work-session notes). Note for reviewing the feature: Blocked-IndirectionThe key algorithmic idea: for loads like IntroductionIndirect-access in array-centric language resembles the pointer-based accesses in C. Given its complexity, pattern based specialization are beneficial for speed of emulator's runtime. Broadly, data mapping between spaces can be considered as reducing/collecting/selecting (large to small spaces), permuting (similar sized spaces), scatter/replicating/broadcasting (small to large spaces). Indirect-access patterns from LLM are mostly fall in large to small spaces or between similar sized spaces. Utilizing the gather vectorized ops from Numpy for realistic sizes of data is natural to boost emulation speed. Given that LLM kernels are native to GPU or matrix-engines that are fluent with dense metrics, arrays and vectors (contrasting to linked-list, graph-traversal and other irregular access patterns), there are not many indirect-carrying kernels appearing in popular LLMs and many of them carry static inner dimensions. Such pattern (the indirection is not in all the subscript expressions) of a KTIR load statement need not to compute indices/addresses of the source space for all the elements under read. Instead, indices of elements (in the source space) in the same static dimension can be produced through Nympy's broadcast op (similar to everyday replication, or splat in SIMDization). For ease of reviewing, many algorithmic, logic considerations are kept in docstrings and comments. Following sections are materials and points useful for the decision making on choices of coding, design, reviewing, and maintenance. Key notes for reviewing:
1. Compliance with PR #179 Review Comments
Summary: 8/9 items fully addressed, 1 justified divergence (gather/scatter kept for perf). 2. Change Set: Blocked-Indirect Path (Current State)Entries and call chainsstages and func codes —
|
| Stage | Function | Size | Role |
|---|---|---|---|
| 0. Helpers | _expr_dependent_vars |
23 | AST walk: extract iteration-dim dependencies |
| 1. Classify | _analyze_blocked_indirect |
78 | Gates 1-3, returns analysis tuple or None |
| 2. Orchestrate/Umbrella | _compute_blocked_indirect_offsets |
25 | Chains stages 2→3→4 |
| 2.1. Prepare subspace | _prepare_dep_var_sub_space |
23 | K sampling coordinates for dep-var subspace |
| 2.2. Read & expand | _runtime_read_and_expand_sub_space |
59 | K index values per sub from HBM (scattered DMA) |
| 2.3. Broadcast | _gen_offsets_vso_space_via_broadcast |
72 | K→N: meshgrid over dep+direct axes → N flat offsets |
| 3. Entry (load) | indirect_load |
48 | Fast path via stage 5, or general path fallback |
| 0. Entry (store) | indirect_store |
49 | Mirror of indirect_load for scatter |
| 377 | Fast path total |
What is a bit new (not in PR 147)
| Addition | Purpose |
|---|---|
Per-sub re-keying in _runtime_read_and_expand_sub_space |
Accumulates per subscription expression, not per view — enables shared-view patterns - Section 7 on complex patterns |
General path (shared infrastructure, also in memory_ops.py)
| Function | Size | Role |
|---|---|---|
_element_offsets |
21 | Reference impl: per-point Python loop |
_enumerate_in_vso_order |
17 | VSO-permuted point enumeration |
_resolve_idx_reads |
17 | General-path wrapper over stage 3 |
_build_indirect_coords |
47 | Per-point coord materialisation |
| 102 | General path total |
Tests
| File | Lines | Tests | Category |
|---|---|---|---|
test_blocked_indirect_fast_path.py |
847 | 35 | Fast path: gating, load, store, equivalence, shared-view, edge cases |
test_indirect_access.py |
889 | 21 | Full stack: MLIR → IAT → indirect_load/store → HBM verification |
| 1736 | 56 |
Benchmark
| File | Lines | Category |
|---|---|---|
bench_indirect_emul_time.py |
256 | Single-purpose: element-wise vs block path, TOML-driven |
Totals by category
| Category | Lines |
|---|---|
| Fast-path implementation | 377 |
| General-path implementation | 102 |
| Tests | 1736 |
| Benchmark | 256 |
| Total | 2471 |
Notes on: _expr_dependent_vars vs eval_subscript_expr
| Function | Purpose | Returns |
|---|---|---|
eval_subscript_expr(expr, pt) |
Evaluate expr at one point | int — the computed value |
_expr_dependent_vars(expr) |
Static analysis: which vars does expr reference? | set — variable indices |
_expr_dependent_vars is purely a block-path concept — it identifies which variables can be factored out (dep_vars → K reads) vs broadcast (direct dims → free amplification). The element-wise path doesn't need this analysis because it evaluates everything at every point anyway (brute-force N iterations).
They both walk the same AST structure, but for different purposes: one computes values, the other extracts metadata.
Rename map (PR 147 → current)
| PR 147 name | Current name | Rationale |
|---|---|---|
_block_gather_analyze |
_analyze_blocked_indirect |
Describes what it does, not the marketing name |
_dep_var_points |
_prepare_dep_var_sub_space |
Clarifies role: defines the dep-var subspace |
_read_idx_at_points |
_runtime_read_and_expand_sub_space |
Clarifies: runtime DMA read that fills the subspace values |
_block_gather_offsets |
_gen_offsets_vso_space_via_broadcast |
Describes the K→N expansion over full VSO space |
_get_blocked_indirect_offsets |
_compute_blocked_indirect_offsets |
Active verb, consistent naming |
_block_gather_load |
(inlined into indirect_load) |
No wrapper needed |
_block_gather_store |
(inlined into indirect_store) |
No wrapper needed |
3. Performance Impact with and without using op gather
| Workload | Current block time | Extra span copy | Block time if reverted | Speedup impact |
|---|---|---|---|---|
| sparse-attn-128K | 1.3 ms | +3.2 ms (32MB copy) | ~4.5 ms | 175× → ~50× |
| sparse-attn-32K | 0.5 ms | +0.4 ms (4MB copy) | ~0.9 ms | 114× → ~63× |
| moe-1M | 9.4 ms | +6.4 ms (64MB copy) | ~16 ms | 117× → ~69× |
| multi-head-4M | 40 ms | +6.4 ms (64MB copy) | ~47 ms | 190× → ~164× |
The sparse-attention workloads get destroyed. The span is 64-128× larger than the data actually needed. Copying the entire parent allocation (a maybe target to treat for future) just to fancy-index a tiny subset is the exact inefficiency that gather eliminates.
Extra span copy column: the cost of reading the entire contiguous allocation into a temp buffer before indexing into it.
- Dense workloads (multi-head, paged-attn with ratio 8×): moderate impact (~15% regression of load time)
- Sparse workloads (sparse-attn with ratio 64-128×): speedup drops by 2-3× (from 175× to ~50× of load time)
gather/scatter is not just "slightly better" — for sparse access patterns, it's the difference between O(selected) and O(parent_size) data movement. The implementation is tiny, ~one line (existing.ravel()[offsets]) and serves the general strided/coords path too (not block-gather-specific duplication).
Hypothetical: Refactoring Gain if gather/scatter Removed (Ignoring Perf)
If we ignore the performance regression, removing gather/scatter would eliminate:
| Deleted | Lines | Location |
|---|---|---|
_gather_from |
~3 | memory.py |
_scatter_into |
~5 | memory.py |
HBMSimulator.gather/scatter |
~12 | memory.py |
LXScratchpad.gather/scatter |
~12 | memory.py |
_MemAccessor.gather/scatter |
~20 | memory_ops.py |
| Total | ~52 lines |
gather/scatter removal: ~52 lines saved, no conceptual simplification, API vs inline code replication, bad for maintainability.
4. lasch's Review Item #1 — _place_in_lx Aliasing
From lasch's PR 147 review:
_place_in_lxaliasing contract is violated — All three call sites produce a C-contiguous alias:data,LX.memory[lx_ptr], andTile.dataall point to the same backing buffer.linalg.matmulandlinalg.batch_matmulboth mutateacc.datain-place with+=, writing through the alias.
His recommendation: "change _place_in_lx to store data.ravel().copy(). Track as a follow-up issue."
Our A3 resolves this exactly — reverted all 4 _place_in_lx call sites to _write_to_lx, which goes through _write_flat → data.flatten() (copy). The aliasing is eliminated. Cost: ~2ms on 40ms block path (190× → 183×).
5. What load Actually Does — Data Flow
HBM dict (np.ndarray) ──fancy-index──► new array ──_write_to_lx──► LX dict
(one copy) ("wire") (second copy)
data.ravel()[offsets]is the copy — allocates output array, fills with selected elements in one pass. No intermediate temp buffer.- The "new array" is a wire — carries the result from the gather unit to the LX write port. No semantic meaning, no lifetime. A Python artifact modeling what hardware does in a single DMA shot.
- On real hardware: one DMA transfer (HBM → LX SRAM with gather unit selecting elements). The simulator models semantics, not the exact transfer mechanism.
6. Motivational patterns: MoE and Paged Attn
The patterns in LLM kernels critical for IBM and community suggests the need for the specialization treatment: majority of the indices into the source of the data load can be calculated in bunks, no need to instantiate element by element. A block-factor, in the table, of 8192× means the fast path reads 8192× fewer index values than the element-wise path would.
| # | Test | Indirection pattern, and block factor |
|---|---|---|
| 1 | test_moe_pattern |
X[IDX[e], m, n], 8 from 128, block_factor=8192x |
| 2 | test_paged_attention_pattern |
cache[BT[0,d0], d1, d2, d3], 1 page, block_factor=16384x |
7. Complex Workload Patterns partially treated
The view-overlap gate rejects any IAT where multiple indirect subs reference the
same index_view_idx. This blocks real workload patterns:
| Pattern | Representation | Why rejected |
|---|---|---|
A[B[i][0], B[i][1], B[i][2], n] |
2D index table, different columns | 3 subs share view B |
A[B[e], B[e+1], m, n] |
1D index array, shifted window | 2 subs share view B |
A[B[e], B[h], m, n] |
1D index, different dep_vars | 2 subs share view B |
A[B[e], B[e], m, n] |
Same expr (degenerate diagonal) | 2 subs share view B |
These patterns appear in: sparse tensor coordinate tables, multi-field block
tables (paged attention), shifted-window attention, multi-dimensional gather.
8. Issue #182: Block-Gather Zero-Padding Bug
1) Error emerging workload/test
Triton-generated variable-length flash-attention prefill kernel:
- Descriptor: 100 rows loaded through a 128-row access tile (fp16, shape
[4, 100, 64], per-head stride100*64=6400) - Final head tile iterates rows 0–127, but only 0–99 exist in the allocation (25,600 elements total)
- Offsets for rows 100–127 compute to indices 25,600+ — past the end of the flat allocation
- Result:
IndexError: index 25600 is out of bounds for axis 0 with size 25600
2) What the spec says about zero-padding
The KTIR spec (RFC 0682) is silent on OOB read semantics. ktdp.load is defined simply as "reads data using an access tile's coordinates; produces a tensor" — no mention of what happens when coordinates fall outside the allocation.
However, the emulator (_read_flat in memory.py:224–255) has an explicit zero-padding contract:
"Elements beyond the end of the containing allocation are zero-padded."
3) Root cause
| Path | OOB handling | Mechanism |
|---|---|---|
_read_flat (strided) |
Zero-pads | Allocates np.zeros(n), copies only available elements |
_gather_from (gather) |
Raises IndexError | Direct data.ravel()[offsets], raw numpy fancy-index — no bounds check, no padding. |
Block-gather computes offsets via numpy broadcast (_gen_offsets_vso_space_via_broadcast). When a partial tile has tail elements mapping past the allocation end, they just point past the physical allocation boundary.
General path pseudo code
# target space allocated and zero-out with the extent (max size)
load_target_space = alloc_zero_out( 128 * 64 )
load_data_to_non_zero_locs(...) --> load_target_space
Block path pseudo code
size_space_indirect_subscripts = 100
subs = load_indirect_subs( size_space_indirect_subscripts )
fancy_index_offsets = numpy.broadcast( subs, 64 )
assert sizeof( fancy_index_offsets ) == 100 * 64
fancy_index_load_data( fancy_index_offsets ) --> load_target_space
# target space size determined by numpy broadcast and allocated to fit the non-loaded data.
Fix (pseudo code)
_gather_from — clamp OOB (out of bound) indices, zero-fill:
flat = data.ravel()
indices = elem_offset + offsets
oob = indices >= flat.size
indices = np.where(oob, 0, indices)
result = flat[indices]
result[oob] = 0 # OOB slots get zero, matching the general path - _read_flat
return result_scatter_into — silently drop OOB writes:
indices = elem_offset + offsets
inbounds = indices < buf.ravel().size
buf.ravel()[indices[inbounds]] = data[inbounds]
lasch
left a comment
There was a problem hiding this comment.
I had the AI rummage through the code and the history of this overall effort since 147 with all the comments and experiments taken into account. Not going to blast this essay here (yet) because it doesn't really reflect the essence (imho).
To me it boils down to 'keep it simple to improve maintenance' vs. 'speed it up to not become the bottleneck' (see the push for a Rust clone). Therefore:
Question: do the benchmarks stand on their own feet? I.e. Could they be extracted into their own PR?
If yes, I'd advice to:
- create a PR with the benchmarks first
- run the benchmarks on the experimental codes that were suggested here
- use the collected evidence to validate the shipped
offsets=design in comparison tocoords=-reuse design
If no: could there be an adjusted (temporary) version of the benchmarks to get numbers from the experimental codes?
|
@lasch great suggestions. acting on it now. |
|
PR #193 has merged. The next steps for this one are now:
|
632c99f to
bb1eba5
Compare
|
[This should have some iterations, I will just post PDF files from some md files.] @lasch comments done. benchmark data for: contrasting table of this branch against fabian's private branch
code rebase + cleanup benchmark |
|
Confirming benchmark results reproduced with roughly the same results. 👍 |
…correctness gap tests - Remove bench_indirect_emul_time.py and indirect_emul.toml (superseded by PR torch-spyre#193) - Add tests for non-zero vss.lo in fast path dep-var iteration (no bug) - Add tests for negative index guard asymmetry between upstream and primitives - Add xfail test for shared-view general-path crash Signed-off-by: Hao Yu <yuh@us.ibm.com>
|
@lasch The final cleanup and soul-searching to avoid surprise like issue #182 are done. Attached is a cut-down version of the report on adding corner case tests. I found it's mostly good and useful. Commit messages:Address PR-188 review: remove superseded benchmarks, add correctness gap tests
On the xfail test targeting an issue in the general-path crashThe detail is in the md file in section tiled as 'Gap 3'. Should be a separate issue. Yet it's quite a corner case. |
The chain of thought is:
-> This is introducing inconsistent behavior depending on the input (the One thing we missed:One proposal from @fabianlim was (and I rephrase): Claude's assessment of this option expects: preserves the exact same hot call and therefore the exact same performance property, while collapsing the primitive-family count from three ( I think, this option has not been tried out or measured/evaluated yet, is that correct @yuhaohaoyu ? I'm pasting the detail about this verbatim without summarization: Addendum: a middle ground for M1/M2, given the G3 performance evidenceM1/M2 keep being flagged as design costs, but G3's data shows the M2 (the parallel Proposal (b) is a different, more aggressive move, and is what the newly-collected G3 data actually measures. Proposal (b) is fabianlim's The two proposals must not be conflated, and only one of them has been measured. Proposal (a) was never built into a branch — it exists only as prose in fabianlim's first comment — so nothing in this PR's benchmark history, including the newly-collected data, has ever measured it. The claim that (a) would preserve the shipped design's performance is an architectural inference (same hot call, no per-point coordinate-tuple materialization, so no obvious new cost) rather than a benchmarked result. It's a reasonably strong inference precisely because (a) looks like a relocation of existing code rather than a different algorithm — unlike (b), which genuinely changes the data path — but it remains a proposal to prototype and benchmark, not a conclusion the evidence collected so far already establishes. What the data does establish is narrower and still useful: (b)'s specific mechanism (coordinate-tuple materialization) has a real sparse-pattern cost, which is one good reason to prefer (a) over (b) if either is pursued — it just isn't yet a demonstration that (a) is free. M1 (the two independent offset-computation pipelines: general per-point enumeration vs. fast K-dedup-then-broadcast) does not have an equally clean middle ground. The K-vs-N dedup is where the dominant 74.5%-of-general-path-time win actually lives (per the G3 step-breakdown analysis), and that's a structurally different mechanism from anything either of fabianlim's proposals touches — both (a) and (b) still read the reduced K-sized point set; they only differ in what happens after. Unifying (checked past comments on issues and PRs for discussion about proposal (a) but didn't find any substantial counter-arguments) |
|
@lasch Addressing for both points are coming in. The benchmarking was great. Here to post the no-regression-on-perf messaging: |
|
@lasch Thanks for the comments. addressed just now. Below are some details to help quick check Architecture changeBefore (3 primitive families, 10 entry points)After (1 primitive family, 6 entry points)
Code Changes
|
| Action | Detail |
|---|---|
| Modified | _read_flat — added *, offsets=None kwarg; sparse branch does OOB→zero-fill |
| Modified | _write_flat — added *, offsets=None kwarg; sparse branch does OOB→drop |
| Deleted | _gather_from (26 lines) |
| Deleted | _scatter_into (24 lines) |
| Modified | HBMSimulator.read / .write — added offsets= forwarding |
| Deleted | HBMSimulator.gather / .scatter (22 lines) |
| Modified | LXScratchpad.read / .write — added offsets= forwarding |
| Deleted | LXScratchpad.gather / .scatter (20 lines) |
ktir_cpu/ops/memory_ops.py
| Action | Detail |
|---|---|
| Modified | _MemAccessor.read / .write — added offsets= forwarding |
| Deleted | _MemAccessor.gather / .scatter (18 lines) |
| Modified | MemoryOps.load branch 1 (offsets) — mgr.gather(...) → mgr.read(..., offsets=) |
| Collapsed | MemoryOps.load branch 3 (coords) — removed span-read + fancy-index; now linearizes coords then calls mgr.read(offsets=) directly |
| Modified | MemoryOps.store branch 1 (offsets) — mgr.scatter(...) → mgr.write(..., offsets=) |
| Simplified | MemoryOps.store branch 3 (coords) — removed read-modify-write; now calls mgr.write(offsets=) directly |
| Modified | distributed_load — mgr.gather(...) → mgr.read(..., offsets=) |
| Modified | distributed_store — mgr.scatter(...) → mgr.write(..., offsets=) |
Performance Verification
$ uv run pytest tests/ -v
========== 1336 passed, 10 skipped, 13 xfailed, 22 warnings in 33.51s ==========
Benchmark (no regression — within run-to-run variance):
Workload | T_indirect_load (before) | T_indirect_load (after)
-----------------+--------------------------+------------------------
moe-262K | 2.45 ms | 2.40 ms
moe-1M | 10.36 ms | 10.18 ms
paged-attn-256K | 34.61 ms | 33.24 ms
sparse-attn-32K | 0.19 ms | 0.17 ms
sparse-attn-128K | 0.29 ms | 0.26 ms
retro-attn-16K | 0.48 ms | 0.41 ms
multi-head-4M | 35.70 ms | 34.02 ms
Overall Change set
- −143 lines net across 3 files (140 insertions, 283 deletions)
- −4 API entry points per layer (10 → 6)
- Branch 3 in
MemoryOps.loadeliminated (coords and offsets paths converge) - Store coord path simplified (no read-modify-write needed)
- OOB semantics unified in one place (
_read_flat/_write_flat)
Comment and test cleanup
Stale references to the deleted gather/scatter/_gather_from/_scatter_into
API were updated in tests/test_blocked_indirect_fast_path.py:
| Old | New |
|---|---|
class TestScatter |
class TestSparseWrite |
"""scatter() writes only the targeted offsets...""" |
"""write(offsets=) writes only the targeted offsets...""" |
test_hbm_scatter_sparse |
test_hbm_write_sparse |
test_lx_scatter_sparse |
test_lx_write_sparse |
test_scatter_gather_roundtrip |
test_write_read_sparse_roundtrip |
class TestGatherScatterOOB |
class TestSparseOOB |
"""Verify that gather zero-pads OOB offsets and scatter drops them.""" |
"""Verify that read(offsets=) zero-pads OOB and write(offsets=) drops them.""" |
test_gather_oob_returns_zero |
test_read_oob_returns_zero |
test_gather_all_inbounds |
test_read_all_inbounds |
test_scatter_oob_dropped |
test_write_oob_dropped |
test_scatter_all_inbounds |
test_write_all_inbounds |
test_gather_primitive_wraps_negative |
test_sparse_read_wraps_negative |
# _gather_from wraps silently (section comment) |
# _read_flat(offsets=) wraps silently |
# gather returns zero ... scatter silently drops (section comment) |
# read(offsets=) returns zero ... write(offsets=) silently drops |
# Scatter primitives: ... (section header) |
# Sparse write primitives: ... |
No tests removed — all test real behavior that still exists under the new API.
shared-view bug fix
The iterator lookup used sub["index_view_idx"] as key into idx_iters, but
idx_values is keyed by subscription index (sub_i). When two subscripts share
the same index_view_idx, both would consume from the same iterator, exhausting
it at ⌈K/2⌉. Fixed with a sequential indirect_counter:
indirect_counter = 0
for sub in iat.dim_subscripts:
if sub["kind"] == "indirect":
raw_idx = int(next(idx_iters[indirect_counter]))
...
indirect_counter += 1|
Thanks for closing this maintainability gap. 👍 One observation was made: 'No test happens to exercise the fast path. The fast path, as currently gated, is unreachable by every existing indirect-access example in the repo, including the paged-attention/paged-tensor ones that read like its intended use case" (all examples fail the I'd say, we can do this separately. There's another maintainability gap raised about the 2 separate code paths that still are separate. M1 — unchanged, not resolved. The general path ( I'd say here too, should be done separately, because: A rough high-level outline of what closing M1 (the two independently-maintained offset-computation pipelines) would take, grounded in the current code in I'm attaching the details as a file: |
|
Thanks @lasch for review comments. I agree to treat the 2 issues as independent and deal with them separately. I am thinking of open another issue to track these, and close the existing feature-dev-driving one. |
|
Yes. Please open new issues for the 2 work items.
|
Factor indirection dims from static dims — read K values, meshgrid to N offsets in one vectorized pass. 100–190× emulation speedup for MoE/paged-attn patterns. Addresses 8/9 PR torch-spyre#179 review items, fixes torch-spyre#182 zero-padding bug. Signed-off-by: Hao Yu <yuh@us.ibm.com>
…correctness gap tests - Remove bench_indirect_emul_time.py and indirect_emul.toml (superseded by PR torch-spyre#193) - Add tests for non-zero vss.lo in fast path dep-var iteration (no bug) - Add tests for negative index guard asymmetry between upstream and primitives - Add xfail test for shared-view general-path crash Signed-off-by: Hao Yu <yuh@us.ibm.com>
…messaging on shared-view handling. - Fold gather/scatter into read/write with offsets= parameter. - Fix shared-view bug in _build_indirect_coords; remove gate-4 rejection Remove _gather_from, _scatter_into, and all .gather()/.scatter() methods Signed-off-by: Hao Yu <yuh@us.ibm.com>
e152e75 to
6930243
Compare
|
@lasch conflict resolved. Please give it a check. Turned out not affected with the HBM --> global renaming, etc. Conflict resolution
Benchmark compare, no regression |
Purpose
Thorough rework of the blocked indirection fast emulation proposal.
Work title: Blocked-indirect fast path: read K indices, broadcast to N offsets
Related issues, PR
Scope of this PR